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

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

Posted by Rayhan Faizel 1 year, 9 months ago

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)

Resolves: https://gitlab.com/libvirt/libvirt/-/issues/14
Signed-off-by: Rayhan Faizel <rayhan.faizel@gmail.com>
---
 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;
-- 
2.34.1
_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Michal Prívozník 1 year, 8 months ago

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@gmail.com>
> ---

... 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@redhat.com>

_______________________________________________
Devel mailing list -- devel@lists.libvirt.org
To unsubscribe send an email to devel-leave@lists.libvirt.org

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

Posted by Rayhan Faizel 1 year, 8 months ago

On Tue, May 14, 2024 at 12:45 PM Michal Prívozník <mprivozn@redhat.com> wrote:
>
> 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@gmail.com>
> > ---
>
> ... 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.
>

This is something I've also noticed. The auto-assigned USB address
does not show up in the x86-64-latest.xml file but appears later in
the args file. Some other examples like usb-audio, usb-mtp, usb disk,
etc. in the test cases seem to exhibit the same behaviour so I didn't
think too much of it. Though from virt-manager's POV, the USB address
does still appear correctly.

> At any rate, your patch is correct.
>
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
>

Sorry for the double mail, forgot to reply to all.

-- 
Rayhan Faizel