On 25.07.2016 22:48, 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>
> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_command.c | 34 ++++++++++++++++++++++++++++----
> src/qemu/qemu_command.h | 3 ++-
> src/qemu/qemu_hotplug.c | 52 ++++++++++++++++++++++++++++++++++++++-----------
> src/qemu/qemu_monitor.c | 21 ++++++++++++++++++++
> src/qemu/qemu_monitor.h | 4 ++++
> 5 files changed, 98 insertions(+), 16 deletions(-)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 31b30a4..6677c28 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4536,12 +4536,14 @@ char *
> qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def,
> virDomainHostdevDefPtr dev,
> virQEMUCapsPtr qemuCaps,
> - virCommandPtr cmd)
> + virCommandPtr cmd,
> + qemuMonitorPtr mon)
> {
> size_t i;
> virBuffer buf = VIR_BUFFER_INITIALIZER;
> virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
> virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &scsisrc->u.vhost;
> + char **vhostfdName = NULL;
> int *vhostfd = NULL;
> size_t vhostfdSize = 1;
>
> @@ -4551,6 +4553,9 @@ qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def,
> goto cleanup;
> }
>
> + if ((cmd && mon) || (!cmd && !mon))
> + goto cleanup;
> +
> if (ARCH_IS_S390(def->os.arch))
> virBufferAddLit(&buf, "vhost-scsi-ccw");
> else
> @@ -4563,18 +4568,28 @@ qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def,
>
> memset(vhostfd, -1, sizeof(*vhostfd) * vhostfdSize);
>
> + if (VIR_ALLOC_N(vhostfdName, vhostfdSize) < 0)
> + goto cleanup;
> +
> if (virSCSIOpenVhost(vhostfd, &vhostfdSize) < 0)
> goto cleanup;
>
> + if (mon) {
> + if (qemuMonitorAttachVhostSCSI(mon, vhostfd, vhostfdName, vhostfdSize) < 0)
> + goto cleanup;
> + }
> +
> for (i = 0; i < vhostfdSize; i++) {
> if (cmd) {
> virCommandPassFD(cmd, vhostfd[i],
> VIR_COMMAND_PASS_FD_CLOSE_PARENT);
> + if (virAsprintf(&vhostfdName[i], "%d", vhostfd[i]) < 0)
> + goto cleanup;
> }
> }
>
> if (vhostfdSize == 1) {
> - virBufferAsprintf(&buf, ",vhostfd=%d", vhostfd[0]);
> + virBufferAsprintf(&buf, ",vhostfd=%s", vhostfdName[0]);
> } else {
> /* FIXME if 'vhostfds' became a valid vhost-scsi property in QEMU */
> goto cleanup;
> @@ -4582,13 +4597,24 @@ qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def,
>
> virBufferAsprintf(&buf, ",id=%s", dev->info->alias);
>
> + for (i = 0; vhostfd && i < vhostfdSize; i++) {
> + if (mon)
> + VIR_FORCE_CLOSE(vhostfd[i]);
> + if (vhostfdName)
> + VIR_FREE(vhostfdName[i]);
> + }
> VIR_FREE(vhostfd);
> + VIR_FREE(vhostfdName);
> return virBufferContentAndReset(&buf);
>
> cleanup:
> - for (i = 0; vhostfd && i < vhostfdSize; i++)
> + for (i = 0; vhostfd && i < vhostfdSize; i++) {
> VIR_FORCE_CLOSE(vhostfd[i]);
> + if (vhostfdName)
> + VIR_FREE(vhostfdName[i]);
> + }
> VIR_FREE(vhostfd);
> + VIR_FREE(vhostfdName);
> virBufferFreeAndReset(&buf);
> return NULL;
> }
While now I understand why you needed the check for cmd == NULL that
I've pointed out in 10/19, this just doesn't feel right.
You have to consider situation where this function succeeds (and sends
some FDs to qemu prematurely, but then something fails later in the
process leaving all the FDs in qemu which will never ever close them.
I'd suggest writing a separate function just to handle the hotplug
instead of bending this one like this. You can take a look at
qemuDomainAttachNetDevice() if you want. The FDs are allocated but not
passed to QEMU until the last moment.
> @@ -5017,7 +5043,7 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
> if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
> if (hostdev->source.subsys.u.scsi.protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) {
> virCommandAddArg(cmd, "-device");
> - if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(def, hostdev, qemuCaps, cmd)))
> + if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(def, hostdev, qemuCaps, cmd, NULL)))
> return -1;
> } else {
> char *drvstr;
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 5c2dcb0..d5a6256 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -163,7 +163,8 @@ char *
> qemuBuildSCSIVhostHostdevDevStr(const virDomainDef *def,
> virDomainHostdevDefPtr dev,
> virQEMUCapsPtr qemuCaps,
> - virCommandPtr cmd);
> + virCommandPtr cmd,
> + qemuMonitorPtr mon);
>
> char *qemuBuildRedirdevDevStr(const virDomainDef *def,
> virDomainRedirdevDefPtr dev,
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 02f7017..77e436c 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2003,16 +2003,29 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
> return -1;
> }
>
> - /* Let's make sure the disk has a controller defined and loaded before
> - * trying to add it. The controller used by the disk must exist before a
> - * qemu command line string is generated.
> - *
> - * Ensure that the given controller and all controllers with a smaller index
> - * exist; there must not be any missing index in between.
> - */
> - for (i = 0; i <= hostdev->info->addr.drive.controller; i++) {
> - if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i))
> - return -1;
> + if (hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) {
> + /* Let's make sure the disk has a controller defined and loaded before
> + * trying to add it. The controller used by the disk must exist before a
> + * qemu command line string is generated.
> + *
> + * Ensure that the given controller and all controllers with a smaller index
> + * exist; there must not be any missing index in between.
> + */
> + for (i = 0; i <= hostdev->info->addr.drive.controller; i++) {
> + if (!qemuDomainFindOrCreateSCSIDiskController(driver, vm, i))
> + return -1;
> + }
> + } else {
> + 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_CCW) {
> + if (virDomainCCWAddressAssign(hostdev->info, priv->ccwaddrs,
This won't fly. Use 'git grep -C10 virDomainCCWAddressAssign' to see how
this function is used.
> + !hostdev->info->addr.ccw.assigned) < 0)
> + goto cleanup;
> + }
> }
>
> if (qemuHostdevPrepareSCSIDevices(driver, vm->def->name,
> @@ -2023,6 +2036,11 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
> virReportError(VIR_ERR_INTERNAL_ERROR,
> _("Unable to prepare scsi hostdev for iSCSI: %s"),
> iscsisrc->path);
> + } else if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) {
> + virDomainHostdevSubsysSCSIVhostPtr vhostsrc = &scsisrc->u.vhost;
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("Unable to prepare scsi hostdev for vhost: %s"),
> + vhostsrc->wwpn);
> } else {
> virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host;
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -2048,7 +2066,19 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn,
> if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0)
> goto cleanup;
>
> - if (hostdev->source.subsys.u.scsi.protocol != VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) {
> + if (hostdev->source.subsys.u.scsi.protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_VHOST) {
> + qemuDomainObjEnterMonitor(driver, vm);
> + if (!(devstr = qemuBuildSCSIVhostHostdevDevStr(vm->def, hostdev, priv->qemuCaps, NULL, priv->mon)))
> + goto cleanup;
> +
> + if (VIR_REALLOC_N(vm->def->hostdevs, vm->def->nhostdevs + 1) < 0)
> + goto cleanup;
> +
> + if ((ret = qemuMonitorAddDevice(priv->mon, devstr)) < 0) {
> + ignore_value(qemuDomainObjExitMonitor(driver, vm) < 0);
> + goto cleanup;
> + }
> + } else {
> if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev)))
> goto cleanup;
>
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 098e654..51c3531 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -2689,6 +2689,27 @@ qemuMonitorGetChardevInfo(qemuMonitorPtr mon,
> }
>
>
> +int
> +qemuMonitorAttachVhostSCSI(qemuMonitorPtr mon,
> + int *vhostfd,
> + char **vhostfdName,
> + int vhostfdSize)
> +{
> + size_t i;
> +
> + QEMU_CHECK_MONITOR(mon);
> +
> + for (i = 0; i < vhostfdSize; i++) {
> + if (virAsprintf(&vhostfdName[i], "vhost-%d", vhostfd[i]) < 0)
> + return -1;
> + if (qemuMonitorSendFileHandle(mon, vhostfdName[i], vhostfd[i]) < 0)
> + return -1;
> + }
> +
> + return 0;
> +}
This function will disappear once the code is reworked following my
suggestions above.
> +
> +
> /**
> * qemuMonitorDriveDel:
> * @mon: monitor object
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index cb4cca8..67050c5 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -679,6 +679,10 @@ void qemuMonitorChardevInfoFree(void *data, const void *name);
> int qemuMonitorGetChardevInfo(qemuMonitorPtr mon,
> virHashTablePtr *retinfo);
>
> +int qemuMonitorAttachVhostSCSI(qemuMonitorPtr mon,
> + int *vhostfd,
> + char **vhostfdName,
> + int vhostfdSize);
> int qemuMonitorAttachPCIDiskController(qemuMonitorPtr mon,
> const char *bus,
> virPCIDeviceAddress *guestAddr);
>
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list