Re: [Xen-devel] [PATCH 8/9] libxl_disk: Use ev_qmp in libxl_cdrom_insert

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH 8/9] libxl_disk: Use ev_qmp in 
libxl_cdrom_insert"):
> libxl_cdrom_insert is now asynchronous when QEMU is involve. And the
> cdrom is now openned by libxl before sending a file descriptor to QEMU.

This patch has much of the meat.  It seems to contain the appropriate
pieces and I generally like it.

I have only minor style quibbles.


I think conventional English grammar in commit messages is to use the
present tense for the situation which exists *before* the commit in
question.  I think you mean:

  Make libxl_cdrom_insert asynchronous when QEMU is involved.  And
  have the cdrom opened by libxl, sending a file descriptor to QEMU.

>          if (rc) goto out;
> +        asynchronous_callback = true;
> +    } else {
> +        asynchronous_callback = false;
...
> -    } else {
> -        cdrom_insert_ejected(egc, cis); /* must be last */
> +    } else if (!asynchronous_callback) {
> +        /* Only called if no asynchronous callback are set. */
> +        cdrom_insert_ejected(egc, &cis->qmp, NULL, 0); /* must be last */

This flag variable is pretty ugly.  Indeed the exit path from this
function is quite fiddly now.  But I can't think of a much prettier
way, and it looks like it is correct to me.

Another possibility would be to move all the variables like t and
d_config into the libxl__cdrom_insert_state, and then the cleanup
would be centralised.  But the lock lifetime of the userdata lock
might be wrong.

So, err, I guess, leave it like this unless you have any better ideas.

> -    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN) {
> -        rc = libxl__qmp_insert_cdrom(gc, domid, disk);
> +    if (cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN &&
> +        disk->format != LIBXL_DISK_FORMAT_EMPTY) {
> +        libxl__json_object *args = NULL;
> +

That this doesn't ever leak payload_fd is not entirely clear.  How
about adding, here

  +        assert(qmp->payload_fd == -1);

?  Since the rule seems to be that the exit path will clean it up, but
that implies that overwriting it might leak a previous fd (of which
there isn't one right now...)

> +        qmp->payload_fd = open(disk->pdev_path, O_RDONLY);
> +        if (qmp->payload_fd < 0) {
> +            LOGED(ERROR, domid, "Failed to open cdrom file %s",
> +                  disk->pdev_path);
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +
> +        /* This free form parameter is not use by QEMU or libxl. */
> +        QMP_PARAMETERS_SPRINTF(&args, "opaque", "%s:%s",
> +                               libxl_disk_format_to_string(disk->format),
> +                               disk->pdev_path);
> +        qmp->callback = cdrom_insert_addfd_cb;
> +        rc = libxl__ev_qmp_send(gc, qmp, "add-fd", args);


Assuming you at least change the commit message, and regardless of
your opinion about the assert:

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

Thanks,
Ian.

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