Re: [Xen-devel] [PATCH v5 15/15] libxl: Re-implement domain_suspend_device_model using libxl__ev_qmp

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v5 15/15] libxl: Re-implement 
domain_suspend_device_model using libxl__ev_qmp"):
> The re-implementation is done because we want to be able to send the
> file description that QEMU can use to save its state. When QEMU is
> restricted, it would not be able to write to a path.

Thanks, this basically LGTM.  I have only very minor comments.

> -static bool qmp_qemu_check_version(libxl__qmp_handler *qmp, int major,
> -                                   int minor, int micro)
> +static bool qmp_ev_qemu_check_version(libxl__ev_qmp *ev, int major,
> +                                      int minor, int micro)
>  {
> -    return qmp->version.major > major ||
> -        (qmp->version.major == major &&
> -            (qmp->version.minor > minor ||
> -             (qmp->version.minor == minor && qmp->version.micro >= micro)));
> +    return ev->qemu_version.major > major ||
> +        (ev->qemu_version.major == major &&
> +            (ev->qemu_version.minor > minor ||
> +             (ev->qemu_version.minor == minor &&
> +              ev->qemu_version.micro >= micro)));

Not your fault, but:

Ow my eyes.  Does this not make you dizzy ?  How about a pre-patch
which replaces this with

#define CHECK(field) do{ \
   if (qmp->version.field > (field)) return +1; \
   if (qmp->version.field < (field)) return -1; \
  }while(0)
   CHECK(major);
   CHECK(minor);
   CHECK(micro);
   return 0;
#undef CHECK

?

Then your actual change here is

 -   if (qmp->version.field > (field)) return +1; \
 -   if (qmp->version.field < (field)) return -1; \
 +   if (ev->qemu_version.field > (field)) return +1; \
 +   if (ev->qemu_version.field < (field)) return -1; \

Or some such.

Up to you, anyway.  While your diff there is hard to read it is at
least making things no worse and the compiler would have spotted
a missed search-and-replace of qmp->version.  You did use
search-and-replace, didn't you ? :-)

> +/*
> + * Function using libxl__ev_qmp
> + */
> +
> +static void dm_stopped(libxl__egc *egc, libxl__ev_qmp *ev,
> +                       const libxl__json_object *response, int rc);
> +static void dm_state_fd_ready(libxl__egc *egc, libxl__ev_qmp *ev,
> +                              const libxl__json_object *response, int rc);
> +static void dm_state_saved(libxl__egc *egc, libxl__ev_qmp *ev,
> +                           const libxl__json_object *response, int rc);
> +int libxl__qmp_suspend_save(libxl__gc *gc, libxl__domain_suspend_state *dsps)

Would you mind adding a blank line just before `int
libxl__qmp_suspend_save' ?

> +    if (rc)
> +        goto error;
> +
> +    libxl__carefd_begin();
> +    ev->cfd = libxl__carefd_opened(CTX,
> +                                 open(filename, O_WRONLY | O_CREAT, 0600));

Does this statefile fd really need to be a carefd ?  Is it a pipe or a
file ?  If it is a file, is it of nontrivial size ?

If it's a file of a few kb, which I think is the case, then the worst
result of (with very low probability) leaking this fd into another
process is simply that the file might be kept alive for a while after
it was deleted.

> +    /* live parameter was added to QEMU 2.11. It signal QEMU that the save

          The `live' parameter                   It signals

> +     * operation is for a live migration rather that for taking a snapshot. 
> */

                                            rather than

> +    if (qmp_ev_qemu_check_version(ev, 2, 11, 0))

If you adopt my proposal above then maybe qmp_ev_qemu_check_version
should be renamed qmp_ev_qemu_compare_version and you would then write

       if (qmp_ev_qemu_compare_version(ev, 2, 11, 0) >= 0) {

> +    if (rc)
> +        goto error;
> +
> +    return;
> +error:

I think a blank line before `error:' would be better.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel