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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, Jul 25, 2024 at 11:31 AM Anthony PERARD
<anthony.perard@xxxxxxxxxx> wrote:
>
> 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.

Thanks for the explanation.  I'll add it here, and also in
cdrom_insert_stubdom_disk_add_cb().

> > +    } 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) {

I'll go with this approach, and that means ...

>
> > +        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.

...this will go away.  I must have re-organized without really
thinking through the result.

> > +        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>

I'll refrain from adding this, since I am making a few more changes.

Thanks for the review.

-Jason