Re: [PATCH v3] libxl: Enable stubdom cdrom changing

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Wed, May 15, 2024 at 10:10:10PM -0400, Jason Andryuk wrote:
> +static void cdrom_insert_stubdom_parse_fdset_rm(libxl__egc *egc,
> +                                                libxl__ev_qmp *qmp,
> +                                                const libxl__json_object 
> *resp,
> +                                                int rc)
> +{
> +    EGC_GC;
> +    libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
> +    int devid;
> +    int fdset;
> +
> +    if (rc) goto out;
> +
> +    /* Only called for qemu-xen/linux stubdom. */
> +    assert(cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN);
> +
> +    devid = libxl__device_disk_dev_number(cis->disk->vdev, NULL, NULL);
> +    fdset = query_fdsets_find_fdset(gc, resp, devid);
> +    if (fdset < 0) {
> +        rc = fdset;
> +        goto out;
> +    }
> +
> +    LOGD(DEBUG, cis->domid, "Found fdset %d", fdset);
> +
> +    libxl__json_object *args = NULL;
> +
> +    libxl__qmp_param_add_integer(gc, &args, "fdset-id", fdset);
> +
> +    cis->qmp.callback = cdrom_insert_stubdom_ejected;
> +
> +    rc = libxl__ev_qmp_send(egc, &cis->qmp, "remove-fd", args);
> +    if (rc) goto out;
> +
> +    return;
> +
> + out:
> +    if (rc == ERROR_NOTFOUND) {
> +        LOGD(DEBUG, cis->domid, "No fdset found - skipping remove-fd");
> +        cdrom_insert_stubdom_ejected(egc, qmp, resp, 0);

I think technically, cdrom_insert_stubdom_ejected also "must be last",
like cdrom_insert_done, for one thing it actually call cdrom_insert_done
in some cases. I think we used "/* must be last */" to indicate that
resources used by the current chain of callback could be freed,
including `egc`, `gc`, `ao`. There's quite a few more calls in this
patch that would benefit from the annotation. But we can live without
those.

> +    } else {
> +        cdrom_insert_done(egc, cis, rc); /* must be last */
> +    }
> +}
[...]
> +static void cdrom_insert_stubdom_parse_fdset(libxl__egc *egc,
> +                                             libxl__ev_qmp *qmp,
> +                                             const libxl__json_object 
> *response,
> +                                             int rc)
> +{
> +    EGC_GC;
> +    libxl__cdrom_insert_state *cis = CONTAINER_OF(qmp, *cis, qmp);
> +    int devid;
> +    int fdset;
> +
> +    if (rc) goto out;
> +
> +    /* Only called for qemu-xen/linux stubdom. */
> +    assert(cis->dm_ver == LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN);
> +
> +    devid = libxl__device_disk_dev_number(cis->disk->vdev, NULL, NULL);
> +    fdset = query_fdsets_find_fdset(gc, response, devid);
> +    if (fdset < 0) {
> +        rc = fdset;
> +        goto out;
> +    }
> +
> +    cis->stubdom_fdset = fdset;
> +
> +    LOGD(DEBUG, cis->domid, "Found fdset %d", cis->stubdom_fdset);
> +    cdrom_insert_ejected(egc, &cis->qmp, NULL, rc);
> +    return;
> +
> + out:
> +    if (rc == ERROR_NOTFOUND) {

While in the previous function it seems ok to deal with the NOTFOUND
error in the "out:" path, I don't think it's a good idea here as it is
an expected part of the workflow of this callback, and not an error.

Could you move setting this timer above again? I guess something like
that would work fine:

  fdset = query_fdsets_find_fdset()
  if (fdset == ERROR_NOTFOUND) {
    // doesn't exist yet, wait a bit
    rc = libxl__ev_time_register_rel()
    if (rc) goto out;
    return
  }
  if (fdset < 0) {
  ...

Or also possible to deal with all error from query_fdsets..() first with
something like:
  if (fdset < 0 && fdset != ERROR_NOTFOUND) {
    rc = fdset;
    goto out;
   ...


> +        rc = libxl__ev_time_register_rel(cis->ao, &cis->retry_timer,
> +                                         cdrom_insert_stubdom_query_fdset,
> +                                         200);
> +        if (rc) goto out;

That "goto out" after the "out" label makes this even stranger and even
a potential infinite loop if `rc` would happen to be set to
ERROR_NOTFOUND again, which I don't think can happen right now.

> +        return;
> +    }
> +
> +    cdrom_insert_done(egc, cis, rc); /* must be last */
> +}


Otherwise patch looks good, with just the comment in
cdrom_insert_stubdom_parse_fdset
act on: Reviewed-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Thanks,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech