Re: [PATCH 1/4] vhost-user support: domain configuration

[Michele Paolino <m.paolino@xxxxxxxxxxxxxxxxxxxxxx>]

On 10/07/2014 18:01, Michal Privoznik wrote:
> On 02.07.2014 15:20, Michele Paolino wrote:
> > vhost-user is added as a virDomainChrSourceDefPtr variable in the
> > virtual network interface configuration structure.
> > This patch adds support to parse vhost-user element. The XML file
> > looks like:
> >
> > <interface type='vhostuser'>
> >       <source type='unix' path='/tmp/vhost.sock' mode='server'/>
> >       <mac address='52:54:00:3b:83:1a'/>
> >       <model type='virtio'/>
> > </interface>
> >
> > Signed-off-by: Michele Paolino <m.paolino@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >    src/conf/domain_conf.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >    src/conf/domain_conf.h | 10 +++++--
> >    2 files changed, 89 insertions(+), 2 deletions(-)
> Introducing a new device (subtype of a device) must always go hand in hand with documentation and XML schema adjustment. Moreover, it would be nice to test the new XML (e.g. domainschematest or qemuxml2argv stub (a .xml file under tests/qemuxml2argvdata without .args counterpart which will be introduced once qemu implementation is done)).

Yes, these files are present in 3/4. I will post the v2 of this series 
in a single patch.

> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> > index ea09bdc..de52ca4 100644
> > --- a/src/conf/domain_conf.c
> > +++ b/src/conf/domain_conf.c
> > @@ -349,6 +349,7 @@ VIR_ENUM_IMPL(virDomainFSWrpolicy, VIR_DOMAIN_FS_WRPOLICY_LAST,
> >    VIR_ENUM_IMPL(virDomainNet, VIR_DOMAIN_NET_TYPE_LAST,
> >                  "user",
> >                  "ethernet",
> > +              "vhostuser",
> >                  "server",
> >                  "client",
> >                  "mcast",
> > @@ -1361,6 +1362,10 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
> >            VIR_FREE(def->data.ethernet.ipaddr);
> >            break;
> >    
> > +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> > +        virDomainChrSourceDefFree(def->data.vhostuser);
> > +        break;
> > +
> >        case VIR_DOMAIN_NET_TYPE_SERVER:
> >        case VIR_DOMAIN_NET_TYPE_CLIENT:
> >        case VIR_DOMAIN_NET_TYPE_MCAST:
> > @@ -6665,6 +6670,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> >        char *mode = NULL;
> >        char *linkstate = NULL;
> >        char *addrtype = NULL;
> > +    char *vhostuser_mode = NULL;
> > +    char *vhostuser_path = NULL;
> > +    char *vhostuser_type = NULL;
> >        virNWFilterHashTablePtr filterparams = NULL;
> >        virDomainActualNetDefPtr actual = NULL;
> >        xmlNodePtr oldnode = ctxt->node;
> > @@ -6710,6 +6718,21 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> >                           xmlStrEqual(cur->name, BAD_CAST "source")) {
> >                    dev  = virXMLPropString(cur, "dev");
> >                    mode = virXMLPropString(cur, "mode");
> > +            } else if (!vhostuser_path && !vhostuser_mode && !vhostuser_type
> > +                       && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
> > +                       xmlStrEqual(cur->name, BAD_CAST "source")) {
> > +                vhostuser_type = virXMLPropString(cur, "type");
> > +                if (!vhostuser_type || STREQ(vhostuser_type, "unix")) {
> > +                    vhostuser_path = virXMLPropString(cur, "path");
> > +                    vhostuser_mode = virXMLPropString(cur, "mode");
> > +                }
> > +                else {
> > +                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> > +                                   _("type='%s' unsupported for"
> > +                                   " <interface type='vhostuser'>"),
> > +                                   vhostuser_type);
> > +                    goto error;
> I'd move this check a few lines below to the other checks.

The check has been done here because if in future we will decide to 
support other chardevs in addition to the unix socket, the XML file will 
be different.

For example, if we want to add the support for a TCP socket, the path 
attribute needs to be replaced with host, service and protocol. After a 
quick look at the _virDomainChrSourceDef structure, the XML description 
in this case should look like:

   <source type='tcp' host='host' service='serv' mode='mode' 
protocol='prot'/>

Do we want to add these additional checks only when the support for 
other chardevs will be added, or is there an alternative solution?

> > +                }
> >                } else if (!def->virtPortProfile
> >                           && xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
> >                    if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> > @@ -6867,6 +6890,50 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> >            actual = NULL;
> >            break;
> >    
> > +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> > +        if (!model || STRNEQ(model, "virtio")) {
> > +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                           _("Wrong or no <model> 'type' attribute "
> > +                           "specified with <interface type='vhostuser'/>. "
> > +                           "vhostuser requires the virtio-net* frontend"));
> > +            goto error;
> > +        }
> > +
> > +        if (VIR_ALLOC(def->data.vhostuser) < 0)
> > +            goto error;
> > +
> > +        if (STREQ(vhostuser_type, "unix")) {
> Ouch, in the code I've commented above, you allow vhostuser_type to be a NULL, so this needs to be STREQ_NULLABLE().
>
> > +
> > +            (def->data.vhostuser)->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> I haven't seen such braces usage since DV stopped writing patches :)
>
> > +
> > +            if (vhostuser_path == NULL) {
> > +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                               _("No <source> 'path' attribute "
> > +                               "specified with <interface "
> > +                               "type='vhostuser'/>"));
> > +                goto error;
> > +            }
> > +
> > +            (def->data.vhostuser)->data.nix.path = vhostuser_path;
> > +
> > +            if (vhostuser_mode == NULL) {
> > +                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                               _("No <source> 'mode' attribute "
> > +                               "specified with <interface "
> > +                               "type='vhostuser'/>"));
> > +                goto error;
> > +            }
> > +
> > +            if (STREQ(vhostuser_mode, "server"))
> > +                (def->data.vhostuser)->data.nix.listen = true;
> Should we disallow other modes? What if user passes:
>
> <interface type='vhostuser'>
>    <source type='unix' path='/some/dummy/path' mode='blah'/>
> </interface>

The nix structure in the _virDomainChrSourceDef uses a boolean to define 
the socket mode. My proposal is to set listen=true only if mode='server' 
and have it equal to false (client) otherwise.

> > +
> > +        }
> > +
> > +        vhostuser_type = NULL;
> > +        vhostuser_path = NULL;
> > +        vhostuser_mode = NULL;
> I don't think you want to set vhostuser_type or vhostuser_mode to NULL here. In case of _path it's okay as you are stealing the pointer into the CharDev struct, but the _type and _mode are not stolen anywhere so you're leaking them effectively.

The (converted) content of vhostuser_type and vhostuser_mode can be 
retrieved in "(def->data.vhostuser)->data.nix.listen" and 
"(def->data.vhostuser)->type".

Do we want their content available anyway?

> > +        break;
> > +
> >        case VIR_DOMAIN_NET_TYPE_ETHERNET:
> >            if (dev != NULL) {
> >                def->data.ethernet.dev = dev;
> > @@ -7112,6 +7179,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
> >        VIR_FREE(portgroup);
> >        VIR_FREE(address);
> >        VIR_FREE(port);
> > +    VIR_FREE(vhostuser_type);
> > +    VIR_FREE(vhostuser_path);
> > +    VIR_FREE(vhostuser_mode);
> >        VIR_FREE(ifname);
> >        VIR_FREE(dev);
> >        virDomainActualNetDefFree(actual);
> > @@ -15989,6 +16059,17 @@ virDomainNetDefFormat(virBufferPtr buf,
> >                                      def->data.ethernet.ipaddr);
> >                break;
> >    
> > +        case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> > +            if ((def->data.vhostuser)->type == VIR_DOMAIN_CHR_TYPE_UNIX) {
> > +                virBufferAddLit(buf, "<source type='unix'");
> > +                virBufferEscapeString(buf, " path='%s'",
> > +                                      (def->data.vhostuser)->data.nix.path);
> > +                if ((def->data.vhostuser)->data.nix.listen)
> > +                    virBufferAddLit(buf, " mode='server'");
> > +                virBufferAddLit(buf, "/>\n");
> > +            }
> > +            break;
> > +
> >            case VIR_DOMAIN_NET_TYPE_BRIDGE:
> >                virBufferEscapeString(buf, "<source bridge='%s'/>\n",
> >                                      def->data.bridge.brname);
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index eb5bad7..91f7524 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -133,6 +133,12 @@ typedef virDomainIdMapDef *virDomainIdMapDefPtr;
> >    typedef struct _virDomainPanicDef virDomainPanicDef;
> >    typedef virDomainPanicDef *virDomainPanicDefPtr;
> >    
> > +/* forward declarations virDomainChrSourceDef, required by
> > + * virDomainNetDef
> > + */
> > +typedef struct _virDomainChrSourceDef virDomainChrSourceDef;
> > +typedef virDomainChrSourceDef *virDomainChrSourceDefPtr;
> > +
> >    /* Flags for the 'type' field in virDomainDeviceDef */
> >    typedef enum {
> >        VIR_DOMAIN_DEVICE_NONE = 0,
> > @@ -795,6 +801,7 @@ struct _virDomainFSDef {
> >    typedef enum {
> >        VIR_DOMAIN_NET_TYPE_USER,
> >        VIR_DOMAIN_NET_TYPE_ETHERNET,
> > +    VIR_DOMAIN_NET_TYPE_VHOSTUSER,
> >        VIR_DOMAIN_NET_TYPE_SERVER,
> >        VIR_DOMAIN_NET_TYPE_CLIENT,
> >        VIR_DOMAIN_NET_TYPE_MCAST,
> This change requires other modifications too. For instance I'm seeing this:
>
>    CC       xenxs/libvirt_xenxs_la-xen_sxpr.lo
> xenxs/xen_sxpr.c: In function 'xenFormatSxprNet':
> xenxs/xen_sxpr.c:1889:5: error: enumeration value 'VIR_DOMAIN_NET_TYPE_VHOSTUSER' not handled in switch [-Werror=switch]
>       switch (def->type) {
>       ^
> cc1: all warnings being treated as errors
>
> and its repetitions in other drivers.
>
> > @@ -880,6 +887,7 @@ struct _virDomainNetDef {
> >                char *dev;
> >                char *ipaddr;
> >            } ethernet;
> > +        virDomainChrSourceDefPtr vhostuser;
> >            struct {
> >                char *address;
> >                int port;
> > @@ -1006,8 +1014,6 @@ typedef enum {
> >    } virDomainChrSpicevmcName;
> >    
> >    /* The host side information for a character device.  */
> > -typedef struct _virDomainChrSourceDef virDomainChrSourceDef;
> > -typedef virDomainChrSourceDef *virDomainChrSourceDefPtr;
> >    struct _virDomainChrSourceDef {
> >        int type; /* virDomainChrType */
> >        union {
> I think this are the bits you need to squash in (don't forget the documentation on RNG schema):
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 4920a4e..1d64f68 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6650,17 +6650,8 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>                          && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
>                          xmlStrEqual(cur->name, BAD_CAST "source")) {
>                   vhostuser_type = virXMLPropString(cur, "type");
> -                if (!vhostuser_type || STREQ(vhostuser_type, "unix")) {
> -                    vhostuser_path = virXMLPropString(cur, "path");
> -                    vhostuser_mode = virXMLPropString(cur, "mode");
> -                }
> -                else {
> -                    virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                                   _("type='%s' unsupported for"
> -                                   " <interface type='vhostuser'>"),
> -                                   vhostuser_type);
> -                    goto error;
> -                }
> +                vhostuser_path = virXMLPropString(cur, "path");
> +                vhostuser_mode = virXMLPropString(cur, "mode");
>               } else if (!def->virtPortProfile
>                          && xmlStrEqual(cur->name, BAD_CAST "virtualport")) {
>                   if (def->type == VIR_DOMAIN_NET_TYPE_NETWORK) {
> @@ -6819,43 +6810,51 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>           break;
>   
>       case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> -        if (!model || STRNEQ(model, "virtio")) {
> +        if (STRNEQ_NULLABLE(model, "virtio")) {
>               virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                              _("Wrong or no <model> 'type' attribute "
> -                           "specified with <interface type='vhostuser'/>. "
> -                           "vhostuser requires the virtio-net* frontend"));
> +                             "specified with <interface type='vhostuser'/>. "
> +                             "vhostuser requires the virtio-net* frontend"));
> +            goto error;
> +        }
> +
> +        if (STRNEQ_NULLABLE(vhostuser_type, "unix")) {
> +            if (vhostuser_type)
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("type='%s' unsupported for"
> +                                 " <interface type='vhostuser'>"),
> +                               vhostuser_type);
> +            else
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                               _("no type specified for "
> +                                 "<interface type='vhostuser'>"));
> +            goto error;
> +        }
> +
> +        if (vhostuser_path == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("No <source> 'path' attribute "
> +                             "specified with <interface "
> +                             "type='vhostuser'/>"));
> +            goto error;
> +        }
> +
> +        if (vhostuser_mode == NULL) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("No <source> 'mode' attribute "
> +                             "specified with <interface "
> +                             "type='vhostuser'/>"));
>               goto error;
>           }
>   
>           if (VIR_ALLOC(def->data.vhostuser) < 0)
>               goto error;
>
> -        if (STREQ(vhostuser_type, "unix")) {
> +        def->data.vhostuser->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> +        def->data.vhostuser->data.nix.path = vhostuser_path;
>   
> -            (def->data.vhostuser)->type = VIR_DOMAIN_CHR_TYPE_UNIX;
> -
> -            if (vhostuser_path == NULL) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("No <source> 'path' attribute "
> -                               "specified with <interface "
> -                               "type='vhostuser'/>"));
> -                goto error;
> -            }
> -
> -            (def->data.vhostuser)->data.nix.path = vhostuser_path;
> -
> -            if (vhostuser_mode == NULL) {
> -                virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -                               _("No <source> 'mode' attribute "
> -                               "specified with <interface "
> -                               "type='vhostuser'/>"));
> -                goto error;
> -            }
> -
> -            if (STREQ(vhostuser_mode, "server"))
> -                (def->data.vhostuser)->data.nix.listen = true;
> -
> -        }
> +        if (STREQ(vhostuser_mode, "server"))
> +            (def->data.vhostuser)->data.nix.listen = true;
>   
>           vhostuser_type = NULL;
>           vhostuser_path = NULL;
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 0b4a0b5..a193a93 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -987,6 +987,7 @@ libxlMakeNic(virDomainDefPtr def,
>               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:
> diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
> index 7a206d2..1d5d689 100644
> --- a/src/uml/uml_conf.c
> +++ b/src/uml/uml_conf.c
> @@ -248,6 +248,11 @@ umlBuildCommandLineNet(virConnectPtr conn,
>                          _("hostdev networking type not supported"));
>           goto error;
>   
> +    case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("vhostuser networking type not supported"));
> +        goto error;
> +
>       case VIR_DOMAIN_NET_TYPE_LAST:
>           break;
>       }
> diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c
> index 9e59804..3d46773 100644
> --- a/src/xenxs/xen_sxpr.c
> +++ b/src/xenxs/xen_sxpr.c
> @@ -1932,6 +1932,7 @@ xenFormatSxprNet(virConnectPtr conn,
>           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:
>
>
> Michal


--
Michele Paolino

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list