Re: [Xen-devel] [PATCH v2 07/15] libxl: convert libxl_domain_destroy to an async op

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH v2 07/15] libxl: convert libxl_domain_destroy 
to an async op"):
> This change introduces some new structures, and breaks the mutual
> dependency that libxl_domain_destroy and libxl__destroy_device_model
> had. This is done by checking if the domid passed to
> libxl_domain_destroy has a stubdom, and then having the bulk of the
> destroy machinery in a separate function (libxl__destroy_domid) that
> doesn't check for stubdom presence, since we check for it in the upper
> level function. The reason behind this change is the need to use
> structures for ao operations, and it was impossible to have two
> different self-referencing structs.

Thanks for what is in general an admirably clear patch for such a
substantial change.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index b13de4f..b44ae75 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
...
> +static void stubdom_callback(libxl__egc *egc, libxl__destroy_domid_state 
> *dis,
> +                             int rc)
> +{
> +    STATE_AO_GC(dis->ao);
> +    libxl__domain_destroy_state *dds = CONTAINER_OF(dis, *dds, stubdom);
> +    const char *savefile;
...
> +    if (dds->domain_finished)
> +        dds->callback(egc, dds, rc);

A suggestion, which you should feel free to reject.  You might like to
concentrate this "have we done all this" logic in one place.  So
instead of the above,

        destroy_finish_check(egc, dds);
    }

    void destroy_finish_check(libxl__egc *egc, libxl__domain_destroy_state *dds)
    {
        if (!(dds->domain_finished &&
              dds->stubdom_finished))
            return;

        dds->callback(egc, dds, dds->rc);
    }

Also thinking about it like this exposes the fact that you aren't
consistent about how to aggregate the rc values from the two destroy
operations.  You give the caller the rc from whichever one finished
last, which doesn't seem like it's correct.

>  /* generic callback for devices that only need to set ao_complete */
> -static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aorm)
> +static void libxl__device_cb(libxl__egc *egc, libxl__ao_device *aodev)
>  {
> -    STATE_AO_GC(aorm->ao);
> +    STATE_AO_GC(aodev->ao);

I think it would be clearer if this use of "aodev" rather than "aorm"
etc. was already in the previous patch, rather than appearing here.
Likewise later in libxl__init_ao_device, etc.

> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index d2c013c..68d076c 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
...
> +_hidden int libxl__ao_device_check_last(libxl__gc *gc, libxl__ao_device 
> *device,
> +                                        libxl__ao_device *list, int num,
> +                                        int *last);
> +

This is a good helper but it could do with a doc comment explaining
what it does.

Just a suggestion: rather than this business with *last it might be
easier to assign a magic error code, or use +1, for "not all done
yet".  (+1 would work since all actual libxl error codes are -ve).
By my reading of the current interface the return value is not
meaningful if *last==0 on output, which is the opposite of the usual
way round.

> @@ -1795,6 +1798,88 @@ struct libxl__ao_device {
>  _hidden void libxl__initiate_device_remove(libxl__egc *egc,
>                                             libxl__ao_device *aorm);
> 
> +/*----- Domain destruction -----*/
> +
> +/* Domain destruction has been splitted in two functions:

"... has been split into two functions:"

> + * libxl__domain_destroy is the main destroy function, it detects

"... function, which detects ..." or "function.  It detects ..."

> + * stubdoms and calls libxl__destroy_domid on the domain and it's

"its stubdom", not "it's"

> + * stubdom if present, creating a different libxl__destroy_domid_state
> + * for each one of them.
...
> +struct libxl__devices_remove_state {
> +    /* filled in by user */
> +    libxl__ao *ao;
> +    uint32_t domid;
> +    libxl__devices_remove_callback *callback;
> +    /* force device destruction */
> +    int force;

"force device destruction" is falling somewhat into the vacuous
comment antipattern.  We know that "force" forces it we knew the
operation was removal.

It would be better to cross-reference this to the public API.  For
example
   int force; /* libxl_device_TYPE_destroy rather than _remove */
and then rely on the public API documentation to describe the
semantics.

> +            printf("device: %s\n", path);
> +            GCNEW(dev);
> +            if (path && libxl__parse_backend_path(gc, path, dev) == 0) {
> +                dev->domid = domid;
> +                dev->kind = kind;
> +                dev->devid = atoi(devs[j]);
> +                drs->aorm[numdev].action = DEVICE_DISCONNECT;
> +                drs->aorm[numdev].dev = dev;
> +                drs->aorm[numdev].callback = device_remove_callback;
> +                drs->aorm[numdev].force = drs->force;

A suggestion: do you want a helper variable
                   libxl__ao_device *aodev = &drs->aorm[numdev];
so that you can write
                   aodev->action = DEVICE_DISCONNECT;
etc. ?

> +static void device_remove_callback(libxl__egc *egc, libxl__ao_device *aorm)
> +{
> +    STATE_AO_GC(aorm->ao);
> +    libxl__devices_remove_state *drs = CONTAINER_OF(aorm->base, *drs, aorm);
> +    char *be_path = libxl__device_backend_path(gc, aorm->dev);
> +    char *fe_path = libxl__device_frontend_path(gc, aorm->dev);
> +    xs_transaction_t t = 0;
> +    int rc = 0, last = 1;
> +
> +retry_transaction:
> +    t = xs_transaction_start(CTX->xsh);
> +    if (aorm->action == DEVICE_DISCONNECT) {
> +        libxl__xs_path_cleanup(gc, t, fe_path);
> +        libxl__xs_path_cleanup(gc, t, be_path);
> +    }
> +    if (!xs_transaction_end(CTX->xsh, t, 0)) {
> +        if (errno == EAGAIN)
> +            goto retry_transaction;
> +        else {
> +            rc = ERROR_FAIL;
> +            goto out;
> +        }
> +    }
> +
> +out:
> +    rc = libxl__ao_device_check_last(gc, aorm, drs->aorm,
> +                                     drs->num_devices, &last);
> +    if (last)
> +        drs->callback(egc, drs, rc);
> +    return;

I don't understand why this is here in this patch.  Surely it belongs
in the previous one ?

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel