[CRIU] [PATCH v2] rework criu check logic

[Saied Kazemi]

On Fri, Mar 11, 2016 at 5:04 AM, Pavel Emelyanov <xemul at virtuozzo.com>
wrote:

> 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
>
>
This is a bit confusing.  On my system (running 3.13 kernel) criu freshly
created from the head will fail both "criu check" and "criu check --ms"
which means I do not have either the new API (prctl(PR_SET_MM,
PR_SET_MM_MAP_SIZE, ...)) or the old API (prctl(PR_SET_MM, PR_SET_MM_BRK,
...)).  However, I can successfully dump and restore a simple main() {
while (1); } C binary.

Why PR_SET_MM_BRK, _EXE_FILE and _AUXV are absolutely required as Category
1 features?

--Saied
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20160311/155bc374/attachment.html>