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

[Roger Pau Monne <roger.pau@citrix.com>]

Ian Jackson wrote:
> 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 ?

disk needs to be added before Qemu is launched, on the other hand nics 
need to be added after qemu is launched, so we need to add them at 
different moments during the domain creation, it's a PITA.

>> +    /* 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 ?

I've added a functions that does the libxl_device_disk_add loop, and 
another one for the nics. Although this line you are quoting is not 
adding a nic, it is just filling the needed values so we can launch Qemu 
correctly.

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