[libvirt] [PATCH 17/17] qemu: support type=hostdev network device hotplug attach/detach

[Laine Stump]

On 03/05/2012 03:46 PM, Michal Privoznik wrote:
> On 28.02.2012 21:14, Laine Stump wrote:
>> qemuDomainAttachNetDevice
>>
>>   - re-ordered some things at start of function because
>>     networkAllocateActualDevice should always be run and a slot
>>     in def->nets always allocated, but host_net_add isn't needed
>>     if the actual type is hostdev.
>>
>>   - if actual type is hostdev, defer to
>>     qemuDomainAttachHostDevice (which will reach up to the NetDef
>>     for things like MAC address when necessary). After return
>>     from qemuDomainAttachHostDevice, slip directly to cleanup,
>>     since the rest of the function is specific to emulated net
>>     devices.
>>
>>   - put assignment of new NetDef into expanded def->nets down
>>     below cleanup: (but only on success) since it is also needed
>>     for emulated and hostdev net devices.
>>
>> qemuDomainDetachHostDevice
>>
>>   - after locating the exact device to detach, check if it's a
>>     network device and, if so, use toplevel
>>     qemuDomainDetachNetDevice instead so that the def->nets list
>>     is properly updated, and 'actual device' properly returned to
>>     network pool if appropriate. Otherwise, for normal hostdevs,
>>     call the lower level qemuDomainDetachThisDevice.
>>
>> qemuDomainDetachNetDevice
>>
>>   - This is where it gets a bit tricky. After locating the device
>>     on the def->nets list, if the network device type == hostdev,
>>     call the *lower level* qemuDomainDetachThisDevice (which will
>>     reach back up to the parent net device for MAC address /
>>     virtualport when appropriate, then clear the device out of
>>     def->hostdevs) before skipping past all the emulated
>>     net-device-specific code to cleanup:, where the network
>>     device is removed from def->nets, and the network device
>>     object is freed.
>>
>> In short, any time a hostdev-type network device is detached, we must
>> go through the toplevel virDomaineDetachNetDevice function first and
>> last, to make sure 1) the def->nnets list is properly managed, and 2)
>> any device allocated with networkAllocateActualDevice is properly
>> freed. At the same time, in the middle we need to go through the
>> lower-level virDomainDetach*This*HostDevice to be sure that 1) the
>> def->hostdevs list is properly managed, 2) the PCI device is properly
>> detached from the guest and reattached to the host (if appropriate),
>> and 3) any higher level setup/teardown is called at the appropriate
>> time, by reaching back up to the NetDef config (part (3) will be
>> covered in a separate patch).
>>
>> ---
>>  src/qemu/qemu_hotplug.c |   61 +++++++++++++++++++++++++++++++++-------------
>>  1 files changed, 44 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 6119108..50563c5 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -661,9 +661,9 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>>      bool iface_connected = false;
>>      int actualType;
>>  
>> -    if (!qemuCapsGet(priv->qemuCaps, QEMU_CAPS_HOST_NET_ADD)) {
>> -        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> -                        _("installed qemu version does not support host_net_add"));
>> +    /* preallocate new slot for device */
>> +    if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets+1) < 0) {
>> +        virReportOOMError();
>>          return -1;
>>      }
>>  
>> @@ -672,9 +672,27 @@ int qemuDomainAttachNetDevice(virConnectPtr conn,
>>       * to the one defined in the network definition.
>>       */
>>      if (networkAllocateActualDevice(net) < 0)
>> -        goto cleanup;
>> +        return -1;
> Okay, vm->def->nets won't leak, but will be one item bigger; Do we want
> to realloc it back as we do in all detach functions, e.g.
> virDomainNetRemove() ? The vm->def->nnets counter isn't changed yet so I
> guess this is alright.

I wondered about this too, but am just copying existing behavior which,
as you say, is relying on the fact that there's no leak, the item is
just one longer (and that one extra in length will be used up the next
time someone wants to grow the array).

>
> Anyway, looking good so ACK.
>

Thanks to you and to Eric for all the reviews!