cr-service: refactor RPC opts parsing for check() #2789
+396
−99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rst0git
force-pushed
the
cr-service-check
branch
from
f0dfdb7 to
2f55575
Compare
|
@rst0git Thank you very much! |
rst0git
force-pushed
the
cr-service-check
branch
from
2f55575 to
3107d83
Compare
avagin
reviewed
Oct 21, 2025
rst0git
force-pushed
the
cr-service-check
branch
2 times, most recently
from
7939b87 to
b3e8429
Compare
This was referenced Oct 25, 2025
herheliuk
reviewed
Oct 25, 2025
herheliuk
reviewed
Oct 25, 2025
rst0git
force-pushed
the
cr-service-check
branch
2 times, most recently
from
df6f7da to
8468539
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@rst0git I'm sorry, but you forged my signature for f1fe4d7 and a18d0b0 commits. Your solution with pre-populating Using 'Co-authored-by' instead would be greatly appreciated. Thank you. |
|
@herheliuk No worries, I was just trying to give you credit for working on this. |
rst0git
force-pushed
the
cr-service-check
branch
from
8468539 to
7c8b5a0
Compare
avagin
reviewed
Oct 27, 2025
avagin
reviewed
Oct 27, 2025
avagin
reviewed
Oct 27, 2025
avagin
reviewed
Oct 27, 2025
avagin
reviewed
Oct 27, 2025
This was referenced Oct 27, 2025
|
This PR is pure MAGIC! ✨🚀 (Maybe 49e9de0 has also played in this, idk) But now I can
from os import makedirs as os_makedirs
os_makedirs('criu_dumps', exist_ok=True)
from pycriu import criu as pycriu_criu
criu = pycriu_criu()
criu.opts.images_dir = 'criu_dumps'
criu.opts.shell_job = True
criu.dump()Can't self-restore using Awesome work, @rst0git Thank you very much! |
rst0git
force-pushed
the
cr-service-check
branch
from
7c8b5a0 to
380840c
Compare
rst0git
force-pushed
the
cr-service-check
branch
from
380840c to
6a5cee6
Compare
@herheliuk I'm not sure if this would be correct. 6fc5266 used to be similar to the patch in #2796. I addressed Andrei's comment above and the patch looks different but the commit message is still very similar. |
|
You're right, it's all good as it is now. Thank you. |
rst0git
force-pushed
the
cr-service-check
branch
2 times, most recently
from
522d349 to
4db71ea
Compare
rst0git
changed the title
rst0git
force-pushed
the
cr-service-check
branch
from
4db71ea to
0be8069
Compare
This change allows users to call criu.use_sk() without any parameters to use the default socket name. Co-authored-by: Radostin Stoyanov <[email protected]> Signed-off-by: Andrii Herheliuk <[email protected]>
Move the code that opens the images directory, resolves its absolute path via readlink(), selects the work_dir, and chdir()s into it into a new function: setup_images_and_workdir(). This reduces the size of `setup_opts_from_req()`, improves its readability, and allows this functionality to be reused. While at it, change open_image_dir() to take a const char *dir parameter, reflecting that the path is not modified by the function and allowing callers to pass string literals without casts. No functional changes are intended. Signed-off-by: Radostin Stoyanov <[email protected]>
Commit 9089ce8 ("service: use setproctitle") extended cr-service to get the full path of images_dir using readlink(). However, the RPC API was later extended to allow setting a custom path (folder) to be set instead of passing a file descriptor, which causes readlink() to fail as the path is not a symbolic link. It would be better to drop the code setting the images-dir path as a string in the proctitle. Fixes: checkpoint-restore#2794 Suggested-by: Andrei Vagin <[email protected]> Co-authored-by: Andrii Herheliuk <[email protected]> Signed-off-by: Radostin Stoyanov <[email protected]>
Move the images_dir selection logic from setup_opts_from_req() into a new function: resolve_images_dir_path(). This improves readability and allows the code to be reused. While at it, use snprintf() instead of sprintf() for the /proc path and ensure NULL termination after strncpy(). Signed-off-by: Radostin Stoyanov <[email protected]>
Move the logging initialization into a helper function that can be reused. No functional change intended. Signed-off-by: Radostin Stoyanov <[email protected]>
rst0git
force-pushed
the
cr-service-check
branch
2 times, most recently
from
45eb439 to
e187cc8
Compare
avagin
reviewed
Oct 28, 2025
|
please don't merge unrelated patches to one pull request. It is much easier to review smaller pr-s |
rst0git
force-pushed
the
cr-service-check
branch
from
e187cc8 to
0b25104
Compare
The check() functionality is very different from dump, pre-dump, and restore. It is used only to check if the kernel supports required features, and does not need the majority of options set via RPC. In particular, we don't need to open `image_dir` when running `check()` because this functionality doesn't create or process image files. In this case, `image_dir` is used as `work_dir`, only when the latter is not specified and a log file is used. This patch updates the RPC options parser so that it only handles the logging options when check() is used. Logging to a file is required when log_file is explicitly set or no log_to_stderr is used. In such case, we also resolve images_dir and work_dir where the log file will be created. Fixes: checkpoint-restore#2758 Suggested-by: Andrei Vagin <[email protected]> Signed-off-by: Radostin Stoyanov <[email protected]>
This allows users to specify RPC options when using the check() functionality. Co-authored-by: Andrii Herheliuk <[email protected]> Signed-off-by: Radostin Stoyanov <[email protected]>
_init__.py defines the public API for pycriu. It is important to use explicit imports to avoid leaking every symbol from criu.py into the pycriu namespace. This avoids import-time side effects, prevents name collisions, and circular-import traps. Fixes the following lint error: F403 `from .criu import *` used; unable to detect undefined names Signed-off-by: Radostin Stoyanov <[email protected]>
The --mntns-compat-mode option is no longer parsed with CHECK. Use --log-file instead to test the error message. Signed-off-by: Radostin Stoyanov <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
Signed-off-by: Radostin Stoyanov <[email protected]>
rst0git
force-pushed
the
cr-service-check
branch
from
0b25104 to
bb41537
Compare
Sorry about this! I've extended |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The check() functionality is very different from
dump,pre-dump, andrestore. It is used only to check if the kernel supports required features, and does not need the majority of options set via RPC.This patch updates the RPC options parser so that it only handles the logging options when
check()is used. In particular, logging to a file is required whenlog_fileis explicitly set or nolog_to_stderris used. In such case, we also resolveimages_dirandwork_dirwhere the log file will be created.Fixes: #2758