8:21 a.m.
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(a)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(a)redhat.com>
Jano