$SUBJ:

s/implement/Implement

On 10/12/18 4:50 AM, Han Han wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1375423
>

Add the infrastructure to allow a USB Hub device to be hot unplugged.

> Signed-off-by: Han Han <hhan@redhat.com>
> ---
>  src/qemu/qemu_driver.c  |  5 ++-
>  src/qemu/qemu_hotplug.c | 74 ++++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_hotplug.h |  4 +++
>  3 files changed, 81 insertions(+), 2 deletions(-)
>

This is where things get a bit dicey. Are you sure we can allow this
given that we automagically create hub devices when there's too many USB
devices, see:

tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.args
tests/qemuxml2argvdata/usb-hub-autoadd-deluxe.xml

and in the code qemuDomainUSBAddressAddHubs?

Note that the test XML doesn't have a hub device defined, but yet some
are created. If someone decides to hot unplug one that has something
plugged into it (e.g. in the case of that test XML, some USB Input
device), then what happens?

Can you test that and ensure the results that you get?

I see you've essentially copied the steps that an Input device would
take; however, I'd suspect a Hub device is a bit more special and we may
need to process the various USB devices to make sure there isn't one
using the Hub device by port... Even worse if it's port=1 and there's a
port=1.1 around that equates to more ports (like from the test).

The question becomes - can we determine which port a USB device is using
via the guest status/live XML? Would the qemu del device error out or
happily accept deleting a hub with attached ports? Or would it just drop
all those attached devices?

Logically what's here would appear to work and is essentially similar to
the Input devices code, so from that aspect things look OK - it's this
one (hah) minor detail.

Tks -

John

> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index de764a7f1c..c8a6d98dc0 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7822,11 +7822,14 @@ qemuDomainDetachDeviceLive(virDomainObjPtr vm,
>          ret = qemuDomainDetachVsockDevice(vm, dev->data.vsock, async);
>          break;
> 
> +    case VIR_DOMAIN_DEVICE_HUB:
> +        ret = qemuDomainDetachHubDevice(vm, dev->data.hub, async);
> +        break;
> +
>      case VIR_DOMAIN_DEVICE_FS:
>      case VIR_DOMAIN_DEVICE_SOUND:
>      case VIR_DOMAIN_DEVICE_VIDEO:
>      case VIR_DOMAIN_DEVICE_GRAPHICS:
> -    case VIR_DOMAIN_DEVICE_HUB:
>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>      case VIR_DOMAIN_DEVICE_NVRAM:
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 1b6cc36bc8..87749ec011 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -4965,6 +4965,32 @@ qemuDomainRemoveRedirdevDevice(virQEMUDriverPtr driver,
>  }
> 
> 
> +static int
> +qemuDomainRemoveHubDevice(virDomainObjPtr vm,
> +                          virDomainHubDefPtr dev)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverPtr driver = priv->driver;
> +    virObjectEventPtr event = NULL;
> +    size_t i;
> +
> +    VIR_DEBUG("Removing hub device %s from domain %p %s",
> +              dev->info.alias, vm, vm->def->name);
> +
> +    event = virDomainEventDeviceRemovedNewFromObj(vm, dev->info.alias);
> +    virObjectEventStateQueue(driver->domainEventState, event);
> +    for (i = 0; i < vm->def->nhubs; i++) {
> +        if (vm->def->hubs[i] == dev)
> +            break;
> +    }
> +    qemuDomainReleaseDeviceAddress(vm, &dev->info, NULL);
> +
> +    virDomainHubDefFree(vm->def->hubs[i]);
> +    VIR_DELETE_ELEMENT(vm->def->hubs, i, vm->def->nhubs);
> +    return 0;
> +}
> +
> +
>  int
>  qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>                         virDomainObjPtr vm,
> @@ -5016,13 +5042,16 @@ qemuDomainRemoveDevice(virQEMUDriverPtr driver,
>          ret = qemuDomainRemoveVsockDevice(vm, dev->data.vsock);
>          break;
> 
> +    case VIR_DOMAIN_DEVICE_HUB:
> +        ret = qemuDomainRemoveHubDevice(vm, dev->data.hub);
> +        break;
> +
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_LEASE:
>      case VIR_DOMAIN_DEVICE_FS:
>      case VIR_DOMAIN_DEVICE_SOUND:
>      case VIR_DOMAIN_DEVICE_VIDEO:
>      case VIR_DOMAIN_DEVICE_GRAPHICS:
> -    case VIR_DOMAIN_DEVICE_HUB:
>      case VIR_DOMAIN_DEVICE_SMARTCARD:
>      case VIR_DOMAIN_DEVICE_MEMBALLOON:
>      case VIR_DOMAIN_DEVICE_NVRAM:
> @@ -6955,3 +6984,46 @@ qemuDomainDetachVsockDevice(virDomainObjPtr vm,
>          qemuDomainResetDeviceRemoval(vm);
>      return ret;
>  }
> +
> +
> +int
> +qemuDomainDetachHubDevice(virDomainObjPtr vm,
> +                          virDomainHubDefPtr def,
> +                          bool async)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virQEMUDriverPtr driver = priv->driver;
> +    virDomainHubDefPtr hub;
> +    int ret = -1;
> +    int idx;
> +
> +    if ((idx = virDomainHubDefFind(vm->def, def)) < 0) {
> +        virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                       _("matching hub device not found"));
> +        return -1;
> +    }
> +    hub = vm->def->hubs[idx];
> +
> +    if (!async)
> +        qemuDomainMarkDeviceForRemoval(vm, &hub->info);
> +
> +    qemuDomainObjEnterMonitor(driver, vm);
> +    if (qemuMonitorDelDevice(priv->mon, hub->info.alias)) {
> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +        goto cleanup;
> +    }
> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +        goto cleanup;
> +
> +    if (async) {
> +        ret = 0;
> +    } else {
> +        if ((ret = qemuDomainWaitForDeviceRemoval(vm)) == 1)
> +            ret = qemuDomainRemoveHubDevice(vm, hub);
> +    }
> +
> + cleanup:
> +    if (!async)
> +        qemuDomainResetDeviceRemoval(vm);
> +    return ret;
> +}
> diff --git a/src/qemu/qemu_hotplug.h b/src/qemu/qemu_hotplug.h
> index 444333c4df..0f205ff54b 100644
> --- a/src/qemu/qemu_hotplug.h
> +++ b/src/qemu/qemu_hotplug.h
> @@ -208,4 +208,8 @@ int qemuDomainDetachInputDevice(virDomainObjPtr vm,
>  int qemuDomainDetachVsockDevice(virDomainObjPtr vm,
>                                  virDomainVsockDefPtr dev,
>                                  bool async);
> +
> +int qemuDomainDetachHubDevice(virDomainObjPtr vm,
> +                              virDomainHubDefPtr def,
> +                              bool async);
>  #endif /* __QEMU_HOTPLUG_H__ */
>