[libvirt] [PATCH 9/9] qemu_hotplug: Support interface type of vhost-user hotplug

[John Ferlan]

On 08/16/2016 11:41 AM, Michal Privoznik wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1366108
> 
> So far, the hotplug of vhost-user "worked" by pure chance. Well,
> qemu would error on our commands, but nevertheless. Now that we
> have everything prepare, We should support hotplugging og

prepared, we ... of

> vhost-user. Firstly, we need to plug in chardev (through which
> qemu talks to OpenVSwitch), then netdev and only after that we
> can plug in the virtio-net-pci device.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/qemu/qemu_hotplug.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index feb1f44..0bcb411 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -933,6 +933,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>      virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>      virDomainCCWAddressSetPtr ccwaddrs = NULL;
>      size_t i;
> +    char *charDevAlias = NULL;
>  
>      /* preallocate new slot for device */
>      if (VIR_REALLOC_N(vm->def->nets, vm->def->nnets + 1) < 0)
> @@ -954,11 +955,11 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>      case VIR_DOMAIN_NET_TYPE_DIRECT:
>      case VIR_DOMAIN_NET_TYPE_BRIDGE:
>      case VIR_DOMAIN_NET_TYPE_HOSTDEV:
> +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>          /* These types are supported. */
>          break;
>  
>      case VIR_DOMAIN_NET_TYPE_USER:
> -    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
>      case VIR_DOMAIN_NET_TYPE_SERVER:
>      case VIR_DOMAIN_NET_TYPE_CLIENT:
>      case VIR_DOMAIN_NET_TYPE_MCAST:
> @@ -1148,6 +1149,26 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>              goto cleanup;
>      }
>  
> +    if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> +        if (!qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           "%s", _("Netdev support unavailable"));

"%s", on previous line

"vhost-user hot-plug not support by this version of qemu"

> +            goto cleanup;
> +        }
> +
> +        if (virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0)
> +            goto cleanup;
> +
> +        qemuDomainObjEnterMonitor(driver, vm);
> +        if (qemuMonitorAttachCharDev(priv->mon, charDevAlias, net->data.vhostuser) < 0) {
> +            ignore_value(qemuDomainObjExitMonitor(driver, vm));
> +            virDomainAuditNet(vm, NULL, net, "attach", false);
> +            goto cleanup;
> +        }
> +        if (qemuDomainObjExitMonitor(driver, vm) < 0)
> +            goto cleanup;
> +    }
> +
>      qemuDomainObjEnterMonitor(driver, vm);
>      if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
>          if (qemuMonitorAddNetdev(priv->mon, netstr,
> @@ -1268,6 +1289,7 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>      }
>      VIR_FREE(vhostfd);
>      VIR_FREE(vhostfdName);
> +    VIR_FREE(charDevAlias);
>      virObjectUnref(cfg);
>      virDomainCCWAddressSetFree(ccwaddrs);
>  
> @@ -1283,6 +1305,9 @@ qemuDomainAttachNetDevice(virQEMUDriverPtr driver,
>              if (virAsprintf(&netdev_name, "host%s", net->info.alias) < 0)
>                  goto cleanup;
>              qemuDomainObjEnterMonitor(driver, vm);
> +            if (charDevAlias &&
> +                qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0)
> +                VIR_WARN("Failed to remove associated chardev %s", charDevAlias);
>              if (qemuMonitorRemoveNetdev(priv->mon, netdev_name) < 0)
>                  VIR_WARN("Failed to remove network backend for netdev %s",
>                           netdev_name);
> @@ -3309,10 +3334,12 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>      virNetDevVPortProfilePtr vport;
>      virObjectEventPtr event;
>      char *hostnet_name = NULL;
> +    char *charDevAlias = NULL;
>      size_t i;
>      int ret = -1;
> +    int actualType = virDomainNetGetActualType(net);
>  
> -    if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
> +    if (actualType == VIR_DOMAIN_NET_TYPE_HOSTDEV) {
>          /* this function handles all hostdev and netdev cleanup */
>          ret = qemuDomainRemoveHostDevice(driver, vm,
>                                           virDomainNetGetActualHostdev(net));
> @@ -3322,9 +3349,11 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>      VIR_DEBUG("Removing network interface %s from domain %p %s",
>                net->info.alias, vm, vm->def->name);
>  
> -    if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0)
> +    if (virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0 ||
> +        virAsprintf(&charDevAlias, "char%s", net->info.alias) < 0)


Part of me wonders if this should be

  ||
  (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
   virAsprintf(&hostnet_name, "host%s", net->info.alias) < 0)

Since it's only necessary for vhost-user, but it does seem excessive.
Then again no more excessive than allocating something we don't use. IDC
either way, but I'd be remiss if I didn't point it out.

>          goto cleanup;
>  
> +
>      qemuDomainObjEnterMonitor(driver, vm);
>      if (qemuDomainSupportsNetdev(vm->def, priv->qemuCaps, net)) {
>          if (qemuMonitorRemoveNetdev(priv->mon, hostnet_name) < 0) {
> @@ -3347,6 +3376,17 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>              goto cleanup;
>          }
>      }
> +
> +    if (actualType == VIR_DOMAIN_NET_TYPE_VHOSTUSER) {
> +        /* vhostuser has a chardev too */
> +        if (qemuMonitorDetachCharDev(priv->mon, charDevAlias) < 0) {
> +            /* well, this is a messy situation. Guest visible PCI device has
> +             * been removed, netdev too but chardev not. The best seems to be
> +             * to just ignore the error and carry on.
> +             */
> +        }
> +    }
> +

The attach order is:

 1. Build 'netstr'
 2. If vhost-user, attach char dev
 3. Add netdev/hostdev using netstr

But your detach order is

 1. Remove netdev/hostdev
 2. Remove vhost-user chardev

See anything wrong with that dependency-wise?

Look at the failure code for Attach - it'll remove CharDev before Netdev

So the question is - if you did this in the right order, then would that
messy situation not exist?


John

>      if (qemuDomainObjExitMonitor(driver, vm) < 0)
>          goto cleanup;
>  
> @@ -3371,7 +3411,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>                                                    &net->mac));
>      }
>  
> -    if (virDomainNetGetActualType(net) == VIR_DOMAIN_NET_TYPE_DIRECT) {
> +    if (actualType == VIR_DOMAIN_NET_TYPE_DIRECT) {
>          ignore_value(virNetDevMacVLanDeleteWithVPortProfile(
>                           net->ifname, &net->mac,
>                           virDomainNetGetActualDirectDev(net),
> @@ -3397,6 +3437,7 @@ qemuDomainRemoveNetDevice(virQEMUDriverPtr driver,
>  
>   cleanup:
>      virObjectUnref(cfg);
> +    VIR_FREE(charDevAlias);
>      VIR_FREE(hostnet_name);
>      return ret;
>  }
>