[libvirt] [PATCH 2/2] qemu: cold-unplug of sound

[Cole Robinson]

On 10/15/19 2:41 AM, Jidong Xia wrote:
> With this patch users can cold unplug some sound devices.
> use "virsh detach-device vm sound.xml --config" command.
> 

I reviewed and pushed patch #1

Some comments below

> Signed-off-by: Jidong Xia <xiajidong at cmss.chinamobile.com>
> ---
>  src/conf/domain_conf.c   | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h   |  2 ++
>  src/libvirt_private.syms |  2 ++
>  src/qemu/qemu_driver.c   |  9 ++++++-
>  4 files changed, 73 insertions(+), 1 deletion(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c1705a0..4083b7c 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -2754,6 +2754,15 @@ void virDomainSmartcardDefFree(virDomainSmartcardDefPtr def)
>      VIR_FREE(def);
>  }
> 
> +virDomainSoundDefPtr
> +virDomainSoundDefRemove(virDomainDefPtr def, size_t idx)
> +{
> +    virDomainSoundDefPtr ret = def->sounds[idx];
> +    VIR_DELETE_ELEMENT(def->sounds, idx, def->nsounds);
> +    return ret;
> + }
> +
> +
>  void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def)
>  {
>      if (!def)
> @@ -17211,6 +17220,58 @@ virDomainNetUpdate(virDomainDefPtr def,
>      return 0;
>  }
> 
> +/**
> + * virDomainSoundFindIdx:
> + * @def: domain definition
> + * @sound: sound definition
> + *
> + * Lookup domain's sound interface based on passed @sound
> + * definition.
> + *
> + * Return: index of match if unique match found,
> + *         -1 otherwise and an error is logged.
> + */
> +
> +int
> +virDomainSoundFindIdx(virDomainDefPtr def, virDomainSoundDefPtr sound)
> +{
> +    size_t i;
> +    int matchidx = -1;
> +    bool PCIAddrSpecified = virDomainDeviceAddressIsValid(&sound->info,
> +                                                          VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI);
> +
> +    for (i = 0; i < def->nsounds; i++) {
> +
> +        if (PCIAddrSpecified &&
> +            !virPCIDeviceAddressEqual(&def->sounds[i]->info.addr.pci,
> +                                      &sound->info.addr.pci))
> +            continue;
> +
> +        if (matchidx >= 0) {
> +            virReportError(VIR_ERR_OPERATION_FAILED, "%s",
> +                           _("multiple matching devices found"));
> +
> +            return -1;
> +        }
> +
> +        matchidx = i;
> +    }
> +
> +    if (matchidx < 0) {
> +        if (PCIAddrSpecified) {
> +            virReportError(VIR_ERR_DEVICE_MISSING,
> +                           _("no device found on %.4x:%.2x:%.2x.%.1x"),
> +                           sound->info.addr.pci.domain,
> +                           sound->info.addr.pci.bus,
> +                           sound->info.addr.pci.slot,
> +                           sound->info.addr.pci.function);
> +        } else {
> +            virReportError(VIR_ERR_DEVICE_MISSING, "%s",
> +                           _("no matching device found"));
> +        }
> +    }
> +    return matchidx;
> +}
> 

I think the model to follow should be virDomainInputDefFind +
virDomainInputDefEquals

* Put all the matching logic in the Equals function
* It should match ->model as well. This way a use could detach just an
XML with <sound model='FOO'/> without requiring the <address> block
* Use virDomainDeviceInfoAddressIsEqual instead of restricting it to
just PCI address. Some day maybe we support usb-audio which would have a
different address type for example

You can CC me on v2 and I'll review it

Thanks,
Cole