Re: [Xen-devel] [PATCH 08/13] libxl: convert libxl_device_disk_add to an async operation

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH 08/13] libxl: convert libxl_device_disk_add to 
an async operation"):

> +static void domcreate_disk_connected(libxl__egc *egc, libxl__ao_device *aorm)
> +{
> +    STATE_AO_GC(aorm->ao);
> +    libxl__domain_create_state *dcs = CONTAINER_OF(aorm->base, *dcs, 
> devices)...
> +    ret = libxl__ao_device_check_last(gc, aorm, dcs->devices,
> +                                      dcs->num_devices, &last);
> +    if (!last) return;
> +    if (last && ret) {
> +        LOGE(ERROR, "error connecting disk devices");
> +        goto error_out;
> +    }

This is a slightly odd way of putting it.  The `if (last' part is
always going to be true because we always return if !last.

Also, please can you use "rc" for libxl error codes rather than "ret".
We should standardise on one name for this and all the recent new code
uses "rc".

> +    /* We might be going to call libxl__spawn_local_dm, or _spawn_stub_dm.
> +     * Fill in any field required by either, including both relevant
> +     * callbacks (_spawn_stub_dm will overwrite our trespass if needed). */
> +    dcs->dmss.dm.spawn.ao = ao;
> +    dcs->dmss.dm.guest_config = dcs->guest_config;
> +    dcs->dmss.dm.build_state = &dcs->build_state;
> +    dcs->dmss.dm.callback = domcreate_devmodel_started;
> +    dcs->dmss.callback = domcreate_devmodel_started;
> +

I don't understand why this hunk appears here in this diff.  Surely
this patch should not need to change the initialisation of dcs->dmss ?

> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 4019309..2b63a15 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
...                                           int rc);
> @@ -800,22 +801,60 @@ retry_transaction:
>          if (errno == EAGAIN)
>              goto retry_transaction;
> 
> +    GCNEW_ARRAY(sdss->devices, dm_config->num_disks);
> +    sdss->num_devices = dm_config->num_disks;
>      for (i = 0; i < dm_config->num_disks; i++) {
> -        ret = libxl_device_disk_add(ctx, dm_domid, &dm_config->disks[i]);
> -        if (ret)
> -            goto out_free;
> +        libxl__init_ao_device(&sdss->devices[i], ao, &sdss->devices);
> +        sdss->devices[i].callback = spawn_stub_disk_connected;
> +        libxl__device_disk_add(egc, dm_domid, &dm_config->disks[i],
> +                               &sdss->devices[i]);
> +    }

We seem to have a couple of instances of this pattern.  You have a
helper function libxl__ao_device_check_last to check they're all done
and fish out the rc value.

Perhaps there should be a small helper function to populate devices[]
and call libxl__device_disk_add ?

Thanks,
Ian.

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