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

[Jidong Xia]

On 11/14/19 5:52 AM, Cole Robinson wrote:
> 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
I see, I accept it.
>
> You can CC me on v2 and I'll review it
ok,I'll send it some days later.
>
> Thanks,
> Cole
>
Thanks,

Jidong Xia