Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net: redirect nftables stdout and stderr to CRIU's log file #2549

Merged

Conversation

adrianreber
Copy link
Member

@adrianreber adrianreber commented Dec 17, 2024

When using the nftables network locking backend and restoring a process a second time the network locking has already been deleted by the first restore. The second restore will print out to the console text like:

Error: Could not process rule: No such file or directory
delete table inet CRIU-202621

With this change CRIU's log FD is used by libnftables stdout and stderr.

criu/net.c Outdated Show resolved Hide resolved
@rst0git
Copy link
Member

rst0git commented Dec 23, 2024

It looks like the changes in this pull request are also applied in #2550
@adrianreber Would it make sense to close this PR?

@adrianreber
Copy link
Member Author

@avagin Anything missing here from your point of view. It is not exactly the same as #2550. It is related but could be done independently of the discussion in #2550

criu/net.c Outdated Show resolved Hide resolved
@adrianreber adrianreber force-pushed the 2024-12-17-nftables-log branch from 4091738 to 79b7ff0 Compare January 15, 2025 11:30
@adrianreber
Copy link
Member Author

Moved everything to a separate function as suggested by @rst0git and removed the fclose() as suggested by @avagin.

criu/net.c Outdated Show resolved Hide resolved
@adrianreber adrianreber force-pushed the 2024-12-17-nftables-log branch from 79b7ff0 to c079a61 Compare January 15, 2025 16:29
@adrianreber adrianreber changed the title net: redirect nftables stdout and stderr to log CRIU's log file net: redirect nftables stdout and stderr to CRIU's log file Jan 15, 2025
Copy link
Member

@rst0git rst0git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@avagin
Copy link
Member

avagin commented Jan 15, 2025

Moved everything to a separate function as suggested by @rst0git and removed the fclose() as suggested by @avagin.

Ooops, I didn't mean that. Without fclose(), it introduces a memory leak, because fdopen allocates a new FILE object. Unfortunately, it is impossible to do fclose() without closing a file description. The only option is to dup a logging file descriptor and operates with a new descriptor.

@adrianreber adrianreber force-pushed the 2024-12-17-nftables-log branch from c079a61 to fd3fea4 Compare January 16, 2025 08:23
@adrianreber
Copy link
Member Author

Moved everything to a separate function as suggested by @rst0git and removed the fclose() as suggested by @avagin.

Ooops, I didn't mean that. Without fclose(), it introduces a memory leak, because fdopen allocates a new FILE object. Unfortunately, it is impossible to do fclose() without closing a file description. The only option is to dup a logging file descriptor and operates with a new descriptor.

Thanks for the explanation. Reworked to close the FILE pointers using a dup()ed FD.

@avagin
Copy link
Member

avagin commented Jan 16, 2025

LGTM. Thanks for working on this.

@avagin
Copy link
Member

avagin commented Jan 16, 2025

Rebasing the commits of this branch on top of the base branch cannot be performed automatically as this would create a different result than a regular merge.

@adrianreber could you rebase it?

@adrianreber
Copy link
Member Author

Rebasing the commits of this branch on top of the base branch cannot be performed automatically as this would create a different result than a regular merge.

@adrianreber could you rebase it?

There is nothing to rebase from what I see. Nothing happens if I try to rebase locally.

@rst0git rst0git force-pushed the 2024-12-17-nftables-log branch 2 times, most recently from 337836d to 91e8a6f Compare January 17, 2025 15:49
Signed-off-by: Adrian Reber <areber@redhat.com>
When using the nftables network locking backend and restoring a process
a second time the network locking has already been deleted by the first
restore. The second restore will print out to the console text like:

Error: Could not process rule: No such file or directory
delete table inet CRIU-202621

With this change CRIU's log FD is used by libnftables stdout and stderr.

Signed-off-by: Adrian Reber <areber@redhat.com>
@rst0git rst0git force-pushed the 2024-12-17-nftables-log branch from 91e8a6f to 1ed4109 Compare January 17, 2025 15:50
@rst0git
Copy link
Member

rst0git commented Jan 17, 2025

@adrianreber GitHub was showing the message This branch cannot be rebased safely and the "Rebase and merge" button was disabled. I rebased the branch on criu-dev and it seems to work now.

@avagin avagin merged commit 27a5b9a into checkpoint-restore:criu-dev Jan 17, 2025
33 of 41 checks passed
@adrianreber adrianreber deleted the 2024-12-17-nftables-log branch January 18, 2025 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants