Re: [PATCH v2] qemu_hotplug: Properly assign USB address to hotplugged usb-net device

On 5/13/24 15:55, Rayhan Faizel wrote: > Previously, the network device hotplug logic would try to ensure only CCW or > PCI addresses. With recent support for the usb-net model, this patch will > ensure USB addresses for usb-net network devices. > > [Changes in v2] > - Additional information in commit message > - Relevant gitlab issue link (mostly already resolved but hotplugging is > mentioned in one of the comments) This changelog belongs ... > > Resolves: https://gitlab.com/libvirt/libvirt/-/issues/14 > Signed-off-by: Rayhan Faizel <rayhan.faizel(a)gmail.com&gt; > --- ... here. Right after these three dashes. The changelong isn't necessary helpful when reading git log, it's needed only for review. > src/qemu/qemu_hotplug.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c > index 774962b0df..3b39941780 100644 > --- a/src/qemu/qemu_hotplug.c > +++ b/src/qemu/qemu_hotplug.c > @@ -1159,8 +1159,11 @@ qemuDomainAttachNetDevice(virQEMUDriver *driver, > goto cleanup; > } > > - if (qemuDomainIsS390CCW(vm->def) && > - net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { > + if (net->model == VIR_DOMAIN_NET_MODEL_USB_NET) { > + if (virDomainUSBAddressEnsure(priv->usbaddrs, &net->info) < 0) > + goto cleanup; > + } else if (qemuDomainIsS390CCW(vm->def) && > + net->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { > net->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW; > if (!(ccwaddrs = virDomainCCWAddressSetCreateFromDomain(vm->def))) > goto cleanup; Right. I was wondering what about the case when a new domain is defined, whether we do fill in address in that case and we do. It's just that our test case doesn't cover that case. I mean, there's tests/qemuxmlconfdata/net-usb.xml and also tests/qemuxmlconfdata/net-usb.x86_64-latest.xml but because the former doesn't have any USB address, it causes qemuDomainAssignUSBAddresses() to return early without assigning any addresses.

At any rate, your patch is correct. Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com&gt;