[CRIU] [PATCH v2] rework criu check logic

[Pavel Emelyanov]

On 03/11/2016 12:01 AM, Saied Kazemi wrote:
> Pavel and Adrian,
> 
> Please take a look at this patch and let me know what you think.
> 
> As you will see, I have changed some pr_error() to pr_warn() when calling
> "criu check --extra" because we're saying that the extra features do not
> necessarily cause dump/restore failures.  But we can't change all pr_error()
> calls to pr_warn() indiscriminately in the functions that are called by
> "criu check --extra" because some of them are real errors.

Well, yes. If you're talking about pr_perror()-s that are called by non cr-check.c
code, we can shut this down by manually lowering the log level.

As far as such replacements in cr-check.c, this seem to be related to which return
code we want to have when extra features checks fail.

> Another item of discussion is the exit code.  Obviously, if there are no
> errors or warnings the exit code has to be zero.  But what if there's a
> warning on an extra or specific feature?
> Currently, the exit code is non-zero but one can argue that for warnings it 
> should still be zero.

I would say it should be non-zero, as user _explicitly_ asks to check for extra
feature, and if it's absent, then the check has failed. And, respectively, the
pr_err/_perror should probably remain such.

> The current patch is a definite improvement (because it was broken) but to make
> it "perfect" will require more time and iterations.  It's also very difficult to 
> test all permutations.

I like the patch and want to merge it, but have only one comment, please, find it
inline:


>     @@ -182,38 +183,13 @@ static int check_prctl(void)
>                     return -1;
>             }
> 
>     -       /*
>     -        * Either new or old interface must be supported in the kernel.
>     -        */
>     -       ret = prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, (unsigned long)&size, 0, 0);
>     -       if (ret) {
>     -               if (!opts.check_ms_kernel) {
>     -                       pr_msg("prctl: PR_SET_MM_MAP is not supported, which "
>     -                              "is required for restoring user namespaces\n");
>     -                       return -1;
>     -               } else
>     -                       pr_warn("Skipping unssuported PR_SET_MM_MAP\n");
>     -
>     -               ret = prctl(PR_SET_MM, PR_SET_MM_BRK, brk(0), 0, 0);
>     -               if (ret) {
>     -                       if (ret == -EPERM)
>     -                               pr_msg("prctl: One needs CAP_SYS_RESOURCE capability to perform testing\n");
>     -                       else
>     -                               pr_msg("prctl: PR_SET_MM is not supported\n");
>     -                       return -1;
>     -               }
>     -
>     -               ret = prctl(PR_SET_MM, PR_SET_MM_EXE_FILE, -1, 0, 0);
>     -               if (ret != -EBADF) {
>     -                       pr_msg("prctl: PR_SET_MM_EXE_FILE is not supported (%d)\n", ret);
>     -                       return -1;
>     -               }
>     -
>     -               ret = prctl(PR_SET_MM, PR_SET_MM_AUXV, (long)&user_auxv, sizeof(user_auxv), 0);
>     -               if (ret) {
>     -                       pr_msg("prctl: PR_SET_MM_AUXV is not supported\n");
>     -                       return -1;
>     -               }

The checks for PR_SET_MM_BRK, _EXE_FILE and _AUXV are still necessary, since they
were added exclusively for CRIU. And CRIU still calls this stuff actually (in user
mode), so presence of these things should be left in Category 1.

-- Pavel