criu: fix error return value

[eriksjolund]

criu_set_freeze_cgroup() returns 0 or -ENOMEM

https://github.com/checkpoint-restore/criu/blob/b17a73b2ee21d64a7cc59968f39e784618837397/lib/c/criu.c#L653

The ENOMEM error is disregarded in this if statement

crun/src/libcrun/container.c

Line 4064 in ab6fd3f

if (UNLIKELY (ret < 0))

This PR inserts a minus (-) so that ENOMEM is not disregarded in the if statement.

I also replaced != with < to protect against the the possibility tha criu_set_freeze_cgroup() in the future is modified to return 1. I'm not sure about this part in the PR. What do you think?

[rst0git]

I also replaced != with < to protect against the the possibility tha criu_set_freeze_cgroup() in the future is modified to return 1. I'm not sure about this part in the PR. What do you think?

I don't think this is necessary. In this case, 0 is used to indicate success, and any other value would indicate an error. We aim to avoid introducing breaking changes in libcriu. If modifications are necessary, it is more likely that we will introduce a new function instead of modifying the return value of an existing one.

[eriksjolund]

The special case I'm considering is this one:

If we assume that both these assumptions are true:

Assumption 1:

criu_set_freeze_cgroup() returns 1 to indicate a failure

Assumption 2:

This patch is applied

--- a/src/libcrun/criu.c
+++ b/src/libcrun/criu.c
@@ -625,7 +625,7 @@ libcrun_container_checkpoint_linux_criu (libcrun_container_status_t *status, lib
 
   ret = libcriu_wrapper->criu_set_freeze_cgroup (freezer_path);
   if (UNLIKELY (ret != 0))
-    return crun_make_error (err, ret, "CRIU: failed setting freezer %d", ret);
+    return crun_make_error (err, -ret, "CRIU: failed setting freezer %d", ret);
 
   /* Set boolean options . */
   libcriu_wrapper->criu_set_leave_running (cr_options->leave_running);

Then crun_make_errror() returns 0

crun/src/libcrun/error.c

Line 62 in ab6fd3f

return -status - 1;

[eriksjolund]

One way to avoid the hypothetical scenario is to modify this PR to be:

--- a/src/libcrun/criu.c
+++ b/src/libcrun/criu.c
@@ -625,7 +625,7 @@ libcrun_container_checkpoint_linux_criu (libcrun_container_status_t *status, lib
 
   ret = libcriu_wrapper->criu_set_freeze_cgroup (freezer_path);
   if (UNLIKELY (ret != 0))
-    return crun_make_error (err, ret, "CRIU: failed setting freezer %d", ret);
+    return crun_make_error (err, 0, "CRIU: failed setting freezer %d", ret);
 
   /* Set boolean options . */
   libcriu_wrapper->criu_set_leave_running (cr_options->leave_running);

[eriksjolund]

I copied this table from https://criu.org/C_API

Return value	errno	Description
0	undefined	Success
>0	undefined	Success (criu_restore() only)
-EBADE	RPC error (if provided(see include/cr-errno.h), 0 otherwise)	RPC has returned fail.
-ECONNREFUSED	errno	Unable to connect to CRIU.
-ECOMM	errno	Unable to send/recv msg to/from CRIU.
-EINVAL	undefined	CRIU doesn't support this type of request. You should probably update
-EBADMSG	undefined	Unexpected response from CRIU. You should probably update CRIU.

[giuseppe]

if >= 0 means success then I think your patch is correct and we report only errors with < 0.

[giuseppe]

LGTM