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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Roger Pau Monne writes ("[PATCH 09/13] libxl: convert libxl_device_nic_add to 
an async operation"):
> This patch converts libxl_device_nic_add to an ao operation that
> waits for device backend to reach state XenbusStateInitWait and then
> marks the operation as completed. This is not really useful now, but
> will be used by latter patches that will launch hotplug scripts after
> we reached the desired xenbus state.

Why do you serialise vif and vbd initialisation ?  Would anything go
wrong if you did them both at once ?

> +    /* Plug nic interfaces */
> +    if (!ret && d_config->num_vifs > 0) {
> +        /* Attach nics */
> +        GCNEW_ARRAY(dcs->devices, d_config->num_vifs);
> +        dcs->num_devices = d_config->num_vifs;
> +        for (i = 0; i < d_config->num_vifs; i++) {
> +            libxl__init_ao_device(&dcs->devices[i], ao, &dcs->devices);
> +            dcs->devices[i].callback = domcreate_nics_connected;
> +            libxl__device_nic_add(egc, domid, &d_config->vifs[i],
> +                                  &dcs->devices[i]);

Ah this pattern again....

> -int libxl__device_nic_add(libxl__gc *gc, uint32_t domid, libxl_device_nic 
> *nic)
> +void libxl__device_nic_add(libxl__egc *egc, uint32_t domid,
> +                           libxl_device_nic *nic, libxl__ao_device *aorm)
>  {
> +    STATE_AO_GC(aorm->ao);
>      flexarray_t *front;
>      flexarray_t *back;
> -    libxl__device device;
> +    libxl__device *device;

You might want to consider a pre-patch which changes this, and
libxl__device_disk_add's, as follows:
  -    libxl__device device;
  +    libxl__device device[1];

That would get rid of the noise from this patch.  Up to you, though;
because it's separated out textually doesn't make the diff unreadable
like it is.

> @@ -845,9 +851,11 @@ static void spawn_stub_disk_connected(libxl__egc *egc, 
> libxl__ao_device *aorm)
>      }
> 
>      for (i = 0; i < dm_config->num_vifs; i++) {
> -        ret = libxl_device_nic_add(ctx, dm_domid, &dm_config->vifs[i]);
> -        if (ret)
> -            goto out;
> +        /* We have to init the nic here, because we still haven't
> +         * called libxl_device_nic_add at this point, but qemu needs
> +         * the nic information to be complete.
> +         */
> +        libxl__device_nic_setdefault(gc, &dm_config->vifs[i]);

This is really too much repetition now.  Can we not have a common "add
devices" function which is called once for the main domain and once
for the stubdom ?

In the future we'll need that if we have more service domains, too.

Thanks,
Ian.

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