Skip to content

Simplify "cpuinfo" subcommand check #2633

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

Open
wants to merge 3 commits into
base: criu-dev
Choose a base branch
from

Conversation

lianakoleva
Copy link

The command "cpuinfo" requires a subcommand "dump" or "check".

  • CR_CPUINFO is removed; CR_CPUINFO_DUMP and CR_CPUINFO_CHECK are added in its place.
  • All parsing is now done in parse_criu_mode, removing need for passing string subcommand to image_dir_mode.
  • Check for valid subcommand is done once at the start with only the enum CR_CPUINFO_DUMP or CR_CPUINFO_CHECK propagated on success, rather than propagating both CR_CPUINFO and the subcommand string while doing strcmp on each call to image_dir_mode.

@avagin
Copy link
Member

avagin commented Mar 26, 2025

CR_CPUINFO is removed; CR_CPUINFO_DUMP and CR_CPUINFO_CHECK are added in its place.

How does it affect existing users who use CR_CPUINFO?

@lianakoleva
Copy link
Author

@avagin Could you share an example?

@avagin
Copy link
Member

avagin commented Mar 27, 2025

@lianakoleva I think I messed up when I was trying to read this code last time. CR_CPUINFO is a completely internal thing and it isn't a part of our public interface.

Copy link

A friendly reminder that this PR had no activity for 30 days.

@avagin avagin removed the stale-pr label May 19, 2025
@avagin avagin self-assigned this May 20, 2025
@avagin
Copy link
Member

avagin commented May 20, 2025

@lianakoleva Sorry for the long delay. The main problem was that I hadn't seen a significant difference between the current and new version. Yesterday, I decided to look at this PR more closely, which led to even more cleanups in crtools.c: lianakoleva#1. Could you please review this change and merge it into your branch?

lianakoleva and others added 3 commits May 23, 2025 08:29
The cpuinfo command requires a "dump" or "check" subcommand. Thus, we
replace `CR_CPUINFO` with `CR_CPUINFO_DUMP` and `CR_CPUINFO_CHECK`.
This allows us to remove unnecessary subcommand check in
`image_dir_mode()` and perform all parsing in `parse_criu_mode()`.

With this change the check for validating the cpuinfo subcommand is
now done only once with `CR_CPUINFO_DUMP` or `CR_CPUINFO_CHECK` enum.

Signed-off-by: Liana Koleva <43767763+lianakoleva@users.noreply.github.com>
Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
Signed-off-by: Andrei Vagin <avagin@gmail.com>
The `criu cpuinfo check` command calls cpu_validate_cpuinfo(), which
attempts to open the cpuinfo.img file using `open_image()`. If the
image file is not found, `open_image()` returns an "empty image"
object. As a result, `cpu_validate_cpuinfo()` tries to read from it
and fails with the following error:

(00.002473) Error (criu/protobuf.c:72): Unexpected EOF on (empty-image)

This patch adds a check for an empty image and appropriate error message.

Signed-off-by: Radostin Stoyanov <rstoyanov@fedoraproject.org>
@rst0git
Copy link
Member

rst0git commented May 23, 2025

@lianakoleva Thank you for working on this!

@avagin I've updated Liana's branch with the cleanups in crtools.c and added one more commit for cpuinfo check to show an error message when the image file is missing.

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