On 09/06/2016 08:58 AM, Eric Farman wrote:
> Adjust the device string that is built for vhost-scsi devices so that it
> can be invoked from hotplug.
>
>>From the QEMU command line, the file descriptors are expect to be numeric only.
> However, for hotplug, the file descriptors are expected to begin with at least
> one alphabetic character else this error occurs:
>
> # virsh attach-device guest_0001 ~/vhost.xml
> error: Failed to attach device from /root/vhost.xml
> error: internal error: unable to execute QEMU command 'getfd':
> Parameter 'fdname' expects a name not starting with a digit
>
> We also close the file descriptor in this case, so that shutting down the
> guest cleans up the host cgroup entries and allows future guests to use
> vhost-scsi devices. (Otherwise the guest will silently end.)
>
> Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 6 ++
> src/qemu/qemu_hotplug.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 154 insertions(+)
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 45b643b..123bbcb 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -13720,6 +13720,12 @@ virDomainHostdevMatchSubsys(virDomainHostdevDefPtr a,
> return virDomainHostdevMatchSubsysSCSIiSCSI(a, b);
> else
> return virDomainHostdevMatchSubsysSCSIHost(a, b);
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> + if (a->source.subsys.u.host.protocol !=
> + b->source.subsys.u.host.protocol)
> + return 0;
> + if (STREQ(a->source.subsys.u.host.wwpn, b->source.subsys.u.host.wwpn))
> + return 1;
> }
> return 0;
> }
This hunk seems to be out of place in this patch... Although I assume
this is because of the remove device searches.
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index e9140a9..5f8d411 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2165,6 +2165,119 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
> goto cleanup;
> }
>
> +static int
> +qemuDomainAttachHostSCSIHostDevice(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainHostdevDefPtr hostdev)
> +{
> + int ret = -1;
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + virDomainCCWAddressSetPtr ccwaddrs = NULL;
> + char *vhostfdName = NULL;
> + int vhostfd = -1;
> + size_t vhostfdSize = 1;
> + char *devstr = NULL;
> + bool teardowncgroup = false;
> + bool teardownlabel = false;
> + bool releaseaddr = false;
> +
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
Here again the capabilities conundrum.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("SCSI passthrough is not supported by this version of qemu"));
> + goto cleanup;
> + }
> +
> + if (qemuSetupHostdevCgroup(vm, hostdev) < 0)
> + goto cleanup;
Oh, um, didn't I comment on this in patch 1 - there's no
VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST in qemuSetupHostdevCgroup
> + teardowncgroup = true;
> +
> + if (virSecurityManagerSetHostdevLabel(driver->securityManager,
> + vm->def, hostdev, NULL) < 0)
> + goto cleanup;
> + teardownlabel = true;
Likewise there's really not much going on...
> +
> + if (virSCSIOpenVhost(&vhostfd, &vhostfdSize) < 0)
> + goto cleanup;
> +
> + if (virAsprintf(&vhostfdName, "vhostfd-%d", vhostfd) < 0)
> + goto cleanup;
> +
> + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
> + if (qemuDomainMachineIsS390CCW(vm->def) &&
> + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_VIRTIO_CCW))
> + hostdev->info->type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW;
> + }
> +
> + if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE ||
> + hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI ) {
> + if (virDomainPCIAddressEnsureAddr(priv->pciaddrs, &hostdev->info) < 0)
'info' is already a virDomainDeviceInfoPtr (unlike where you
cut-n-paste'd from), so no need for the '&' here (besides it induces a
build failure).
> + goto cleanup;
> + } else if (hostdev->info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) {
> + if (!(ccwaddrs = qemuDomainCCWAddrSetCreateFromDomain(vm->def)))
> + goto cleanup;
> + if (virDomainCCWAddressAssign(hostdev->info, ccwaddrs,
> + !hostdev->info->addr.ccw.assigned) < 0)
> + goto cleanup;
> + }
> + releaseaddr = true;
> +
> + if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0)
> + goto cleanup;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + if (hostdev->source.subsys.u.host.protocol == VIR_DOMAIN_HOSTDEV_HOST_PROTOCOL_TYPE_VHOST) {
Long line > 80 cols
Ironically we do nothing if the protocol isn't VHOST...
> + if (!(devstr = qemuBuildHostHostdevDevStr(vm->def,
> + hostdev,
> + priv->qemuCaps,
> + vhostfdName,
> + vhostfdSize)))
This can be done outside holding the monitor lock
> + goto exit_monitor;
> +
> + if ((ret = qemuMonitorAddDeviceWithFd(priv->mon,
> + devstr,
> + vhostfd,
> + vhostfdName)) < 0) {
> + goto exit_monitor;
> + }
> + }
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + goto cleanup;
> +
> + virDomainAuditHostdev(vm, hostdev, "attach", true);
s/true/(ret == 0)/
Since we've set up the addresses above - the auditing code should print
more than "?"
Here should be a "if (ret < 0) goto cleanup;" (on two lines).
> +
> + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
> + goto cleanup;
> +
> + vm->def->hostdevs[vm->def->nhostdevs++] = hostdev;
Other code will increase the size of hostdevs before the monitor calls,
then just insert.
> +
> + virDomainCCWAddressSetFree(ccwaddrs);
> +
> + VIR_FORCE_CLOSE(vhostfd);
This I would think would only be done on *failure* to AddDeviceWithFd,
but I see you're following qemuDomainAttachHostPCIDevice
> + VIR_FREE(vhostfdName);
> + VIR_FREE(devstr);
> + return 0;
> +
> + exit_monitor:
> + ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +
> + cleanup:
> + if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0)
> + VIR_WARN("Unable to remove host device cgroup ACL on hotplug fail");
> + if (teardownlabel &&
> + virSecurityManagerRestoreHostdevLabel(driver->securityManager,
> + vm->def, hostdev, NULL) < 0)
> + VIR_WARN("Unable to restore host device labelling on hotplug fail");
> + if (releaseaddr)
> + qemuDomainReleaseDeviceAddress(vm, hostdev->info, NULL);
> +
> + VIR_FORCE_CLOSE(vhostfd);
> + VIR_FREE(vhostfdName);
> + VIR_FREE(devstr);
> + return ret;
> +}
> +
>
> int
> qemuDomainAttachHostDevice(virConnectPtr conn,
> @@ -2198,6 +2311,11 @@ qemuDomainAttachHostDevice(virConnectPtr conn,
> goto error;
> break;
>
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> + if (qemuDomainAttachHostSCSIHostDevice(driver, vm, hostdev) < 0)
> + goto error;
> + break;
> +
> default:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("hostdev subsys type '%s' not supported"),
> @@ -3960,6 +4078,31 @@ qemuDomainDetachHostSCSIDevice(virQEMUDriverPtr driver,
> }
>
> static int
> +qemuDomainDetachHostSCSIHostDevice(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + virDomainHostdevDefPtr detach)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> + int ret = -1;
> +
> + if (!detach->info->alias) {
> + virReportError(VIR_ERR_OPERATION_FAILED,
> + "%s", _("device cannot be detached without a device alias"));
> + return -1;
> + }
> +
> + qemuDomainMarkDeviceForRemoval(vm, detach->info);
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> + ret = qemuMonitorDelDevice(priv->mon, detach->info->alias);
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0)
> + return -1;
> +
> + return ret;
> +}
> +
> +static int
> qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> virDomainHostdevDefPtr detach)
> @@ -3981,6 +4124,9 @@ qemuDomainDetachThisHostDevice(virQEMUDriverPtr driver,
> case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI:
> ret = qemuDomainDetachHostSCSIDevice(driver, vm, detach);
> break;
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
> + ret = qemuDomainDetachHostSCSIHostDevice(driver, vm, detach);
> + break;
> default:
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("hostdev subsys type '%s' not supported"),
> @@ -4058,6 +4204,8 @@ int qemuDomainDetachHostDevice(virQEMUDriverPtr driver,
Ah - and this is where that pesky virDomainHostdevFind causes the need
for the domain_conf.c code above...
> }
> break;
> }
> + case VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_HOST:
But we need an error here otherwise we get a generic failed with some
error message.
John
> + break;
> default:
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("unexpected hostdev type %d"), subsys->type);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list