[libvirt] [PATCH 05/21] qemu_hotplug: eliminate multiple identical qemuDomainDetachHost*Device() functions
Laine Stump
laine at laine.org
Fri Mar 22 13:21:38 UTC 2019
On 3/22/19 8:28 AM, Ján Tomko wrote:
> On Thu, Mar 21, 2019 at 06:28:45PM -0400, Laine Stump wrote:
>> There are separate Detach functions for PCI, USB, SCSI, Vhost, and
>> Mediated hostdevs, but the functions are all 100% the same code,
>> except that the PCI function checks for the guest side of the device
>> being a PCI Multifunction device, while the other 4 check that the
>> device's alias != NULL.
>>
>> The check for multifunction PCI devices should be done for *all*
>> devices that are connected to the PCI bus in the guest, not just PCI
>> hostdevs, and qemuIsMultiFunctionDevice() conveniently returns false
>> if the queried device doesn't connect with PCI, so it is safe to make
>> this check for all hostdev devices. (It also needs to be done for many
>> other device types, but that will be addressed in a future patch).
>>
>> Likewise, since all hostdevs are detached by calling
>> qemuDomainDeleteDevice(), which requires the device's alias, checking
>> for a valid alias is a reasonable thing for PCI hostdevs too (NB:
>> you'd think that we could rely on every device having a valid alias,
>> but unfortunately when you run virsh qemu-attach on a qemu process
>> that was started with some "old style" device args, they don't include
>> an "id" option on the commandline, so there is no alias in the device
>> object that's created).
>>
>
> Wrong.
>
> As of
> commit e3a52afcfcc3478b553dca38140394fd93f90a2c
> qemu-attach: Assign device aliases
Ah, I stand corrected. I had looked at the earlier patch in that series,
but skipped this final patch because I'd already found what I wanted.
> that is not true.
>
> Although other than not crashing later, I'm not sure what the point of
> making
> up aliases that don't match reality is.
That thing you said - "not crashing later", that's about it :-P
> Or what the point of qemu-attach is anymore - it does not seem to parse
> -device arguments, nor is it able to get us a JSON monitor, so we would
> not be able to detach the device anyway.
>
>> Signed-off-by: Laine Stump <laine at laine.org>
>> ---
>> src/qemu/qemu_hotplug.c | 129 +++++++++-------------------------------
>> 1 file changed, 28 insertions(+), 101 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 701458b2cd..389d16b090 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -5499,124 +5499,42 @@ int
>> qemuDomainDetachControllerDevice(virQEMUDriverPtr driver,
>> return ret;
>> }
>>
>> -static int
>> -qemuDomainDetachHostPCIDevice(virDomainObjPtr vm,
>> - virDomainHostdevDefPtr detach,
>> - bool async)
>> -{
>> - virDomainHostdevSubsysPCIPtr pcisrc = &detach->source.subsys.u.pci;
>> -
>> - if (qemuIsMultiFunctionDevice(vm->def, detach->info)) {
>> - virReportError(VIR_ERR_OPERATION_FAILED,
>> - _("cannot hot unplug multifunction PCI
>> device: %.4x:%.2x:%.2x.%.1x"),
>> - pcisrc->addr.domain, pcisrc->addr.bus,
>> - pcisrc->addr.slot, pcisrc->addr.function);
>> - return -1;
>> - }
>> -
>> - if (!async)
>> - qemuDomainMarkDeviceForRemoval(vm, detach->info);
>> -
>> - return qemuDomainDeleteDevice(vm, detach->info->alias);
>> -}
>>
>> static int
>> -qemuDomainDetachHostUSBDevice(virDomainObjPtr vm,
>> - virDomainHostdevDefPtr detach,
>> - bool async)
>> -{
>> - if (!detach->info->alias) {
>> - virReportError(VIR_ERR_OPERATION_FAILED,
>> - "%s", _("device cannot be detached without a
>> device alias"));
>> - return -1;
>> - }
>> -
>> - if (!async)
>> - qemuDomainMarkDeviceForRemoval(vm, detach->info);
>> -
>> - return qemuDomainDeleteDevice(vm, detach->info->alias);
>> -}
>> -
>> -static int
>> -qemuDomainDetachHostSCSIDevice(virDomainObjPtr vm,
>> +qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
>> + virDomainObjPtr vm,
>> virDomainHostdevDefPtr detach,
>> bool async)
>> {
>> - if (!detach->info->alias) {
>> - virReportError(VIR_ERR_OPERATION_FAILED,
>> - "%s", _("device cannot be detached without a
>> device alias"));
>> - return -1;
>> - }
>> -
>> - if (!async)
>> - qemuDomainMarkDeviceForRemoval(vm, detach->info);
>> + int ret = -1;
>>
>> - return qemuDomainDeleteDevice(vm, detach->info->alias);
>> -}
>> + if (qemuAssignDeviceHostdevAlias(vm->def, &detach->info->alias,
>> -1) < 0)
>> + return -1;
>
> Here you leave in the bogus alias assignment introduced by
> 9de26f27cfa6a32ce9a23e30a58991432bdcbee5
> hotplug: Check for alias in hostdev detach
Yeah, *that* is the patch I found when I was looking for the source of
the lines.
The reason both the "check for no alias" and "assign an alias if it
isn't there" are in this patch is because I was trying to keep it to
just having code movement, with no deletion, just to make review so
simple it could be done by a mythical code review robot. But since both
you and Peter called it out, I guess that was unnecessary, and I'll
happily remove it.
>
>>
>> -static int
>> -qemuDomainDetachSCSIVHostDevice(virDomainObjPtr vm,
>> - virDomainHostdevDefPtr detach,
>> - bool async)
>> -{
>> - if (!detach->info->alias) {
>> + if (qemuIsMultiFunctionDevice(vm->def, detach->info)) {
>> virReportError(VIR_ERR_OPERATION_FAILED,
>> - "%s", _("device cannot be detached without a
>> device alias"));
>> + _("cannot hot unplug multifunction PCI device
>> with guest address: "
>> + "%.4x:%.2x:%.2x.%.1x"),
>> + detach->info->addr.pci.domain,
>> detach->info->addr.pci.bus,
>> + detach->info->addr.pci.slot,
>> detach->info->addr.pci.function);
>> return -1;
>> }
>>
>> - if (!async)
>> - qemuDomainMarkDeviceForRemoval(vm, detach->info);
>> -
>> - return qemuDomainDeleteDevice(vm, detach->info->alias);
>> -}
>> -
>> -
>> -static int
>> -qemuDomainDetachMediatedDevice(virDomainObjPtr vm,
>> - virDomainHostdevDefPtr detach,
>> - bool async)
>> -{
>> if (!detach->info->alias) {
>> - virReportError(VIR_ERR_OPERATION_FAILED, "%s",
>> - _("device cannot be detached without a device
>> alias"));
>> + virReportError(VIR_ERR_OPERATION_FAILED,
>> + "%s", _("device cannot be detached without a
>> device alias"));
>> return -1;
>> }
>
> which makes this check pointless.
Yeah, that will be removed later anyway. The only reason it showed up in
the diff is because I apparently somehow reformatted it. I'll
"unreformat" it so it doesn't show up in the diff.
>
>>
>> - if (!async)
>> - qemuDomainMarkDeviceForRemoval(vm, detach->info);
>> -
>> - return qemuDomainDeleteDevice(vm, detach->info->alias);
>> -}
>> -
>> -
>> -static int
>> -qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
>> - virDomainObjPtr vm,
>> - virDomainHostdevDefPtr detach,
>> - bool async)
>> -{
>> - int ret = -1;
>> -
>> - if (qemuAssignDeviceHostdevAlias(vm->def, &detach->info->alias,
>> -1) < 0)
>> - return -1;
>> -
>> switch (detach->source.subsys.type) {
>
> Unrelated to this patch, but it seems this could benefit from a typecast
> and an EnumError.
True Dat. (Sorry for the "youngster speak", but I'm trying to compensate
for the picture on my new driver's license - I just got it renewed, and
comparing the picture with the one taken 7 years ago, I look like a
frumpy old man (in the old license, I looked like (in the words of my my
beloved daughter) "a psycho killer", but at least I looked like a
*youthful* psycho killer :-/)
>
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_PCI:
>> - ret = qemuDomainDetachHostPCIDevice(vm, detach, async);
>> - break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_USB:
>> - ret = qemuDomainDetachHostUSBDevice(vm, detach, async);
>> - break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
>> - ret = qemuDomainDetachHostSCSIDevice(vm, detach, async);
>> - break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI_HOST:
>> - ret = qemuDomainDetachSCSIVHostDevice(vm, detach, async);
>> - break;
>> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_MDEV:
>> - ret = qemuDomainDetachMediatedDevice(vm, detach, async);
>> - break;
>> + /* we support detach of all these types of hostdev */
>> + break;
>> +
>> default:
>> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> _("hot unplug is not supported for hostdev
>> subsys type '%s'"),
>
> With the qemu-attach aliaslessness allegations retracted
Well, I'll reword it at least. I think there still should be a check for
a null alias, just in case someone in the future adds some stupid code
that doesn't set an alias, but I'll make the comment more accurate.
> and the double
> check for alias removed:
> Reviewed-by: Ján Tomko <jtomko at redhat.com>
>
> Jano
More information about the libvir-list
mailing list