[libvirt] [PATCH 5/8] qemu: implement vsock hotplug

[John Ferlan]

On 05/30/2018 10:57 AM, Ján Tomko wrote:
> Allow hotplugging the vsock device.
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1291851
> 
> Signed-off-by: Ján Tomko <jtomko at redhat.com>
> ---
>  src/qemu/qemu_driver.c  |  9 ++++++-
>  src/qemu/qemu_hotplug.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_hotplug.h |  4 +++
>  3 files changed, 82 insertions(+), 1 deletion(-)
> 

Why is QEMU_CAPS_DEVICE_VHOST_VSOCK never checked in the command line
startup code?  Was that forgotten or just felt not needed.


> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 3aa694de12..fa94ae9e38 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7690,6 +7690,14 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>          }
>          break;
>  
> +    case VIR_DOMAIN_DEVICE_VSOCK:
> +        ret = qemuDomainAttachVsockDevice(driver, vm, dev->data.vsock);
> +        if (ret == 0) {
> +            alias = dev->data.vsock->info.alias;
> +            dev->data.vsock = NULL;
> +        }
> +        break;
> +
>      case VIR_DOMAIN_DEVICE_NONE:
>      case VIR_DOMAIN_DEVICE_FS:
>      case VIR_DOMAIN_DEVICE_SOUND:
> @@ -7702,7 +7710,6 @@ qemuDomainAttachDeviceLive(virDomainObjPtr vm,
>      case VIR_DOMAIN_DEVICE_TPM:
>      case VIR_DOMAIN_DEVICE_PANIC:
>      case VIR_DOMAIN_DEVICE_IOMMU:
> -    case VIR_DOMAIN_DEVICE_VSOCK:
>      case VIR_DOMAIN_DEVICE_LAST:
>          virReportError(VIR_ERR_OPERATION_UNSUPPORTED,
>                         _("live attach of device '%s' is not supported"),
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index b4bbe62c75..ada120bcfe 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -3015,6 +3015,76 @@ qemuDomainAttachInputDevice(virQEMUDriverPtr driver,
>  }
>  
>  
> +int
> +qemuDomainAttachVsockDevice(virQEMUDriverPtr driver,
> +                            virDomainObjPtr vm,
> +                            virDomainVsockDefPtr vsock)
> +{
> +    qemuDomainVsockPrivatePtr vsockPriv = (qemuDomainVsockPrivatePtr)vsock->privateData;
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virDomainDeviceDef dev = { VIR_DOMAIN_DEVICE_VSOCK,
> +                               { .vsock = vsock } };
> +    virErrorPtr originalError = NULL;
> +    const char *fdprefix = "vsockfd";
> +    bool releaseaddr = false;
> +    char *fdname = NULL;
> +    char *devstr = NULL;
> +    int ret = -1;
> +
> +    if (vm->def->vsock) {
> +        virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> +                       _("the domain already has a vsock device"));
> +        return -1;
> +    }

Theoretically, shouldn't this code check if QEMU_CAPS_DEVICE_VHOST_VSOCK
is available for the domain?

qemuDomainAttachSCSIVHostDevice and qemuDomainAttachHostPCIDevice (other
consumers of qemuMonitorAddDeviceWithFd have capabilities checks...)

> +
> +    if (qemuDomainEnsureVirtioAddress(&releaseaddr, vm, &dev, "vsock") < 0)
> +        return -1;
> +
> +    if (qemuAssignDeviceVsockAlias(vsock) < 0)
> +        goto cleanup;
> +
> +    if (qemuProcessOpenVhostVsock(vsock) < 0)
> +        goto cleanup;
> +
> +    if (virAsprintf(&fdname, "%s%u", fdprefix, vsockPriv->vhostfd) < 0)

Should this be similar to others using "fd-%d" and "vhost-%d"...  IDC,
but this is why I made the comment in patch 2 review.

With at least the capability check added or an explanation why it
doesn't need to be,

Reviewed-by: John Ferlan <jferlan at redhat.com>

John


[...]