Re: [libvirt] [PATCH v2 04/11] conf: net: Add model enum, and netfront value

On 3/13/19 4:51 PM, Cole Robinson wrote: > This adds a network model enum. The virDomainNetDef property > is named 'model' like most other devices. > > When the XML parser or a driver calls NetSetModelString, if > the passed string is in the enum, we will set net->model, > otherwise we copy the string into net->modelstr > > Add a single example for the 'netfront' xen model, and wire > that up, just to verify it's all working > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > --- >   src/conf/domain_conf.c     | 55 +++++++++++++++++++++++++++----------- >   src/conf/domain_conf.h     | 10 +++++++ >   src/libvirt_private.syms   |  2 ++ >   src/libxl/libxl_conf.c     |  4 +-- >   src/qemu/qemu_hotplug.c    |  8 ++++++ >   src/xenconfig/xen_common.c | 16 +++++------ >   src/xenconfig/xen_sxpr.c   | 15 ++++++----- >   7 files changed, 78 insertions(+), 32 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index fe1945b644..571f2eea39 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -472,6 +472,11 @@ VIR_ENUM_IMPL(virDomainNet, > VIR_DOMAIN_NET_TYPE_LAST, >                 "udp", >   ); >   +VIR_ENUM_IMPL(virDomainNetModel, VIR_DOMAIN_NET_MODEL_LAST, This is supposed to be on a separate line now ;-) > +              "unknown", > +              "netfront", > +); > + >   VIR_ENUM_IMPL(virDomainNetBackend, VIR_DOMAIN_NET_BACKEND_TYPE_LAST, >                 "default", >                 "qemu", > @@ -2228,6 +2233,7 @@ virDomainNetDefClear(virDomainNetDefPtr def) >           return; >         VIR_FREE(def->modelstr); > +    def->model = VIR_DOMAIN_NET_MODEL_UNKNOWN; >         switch (def->type) { >       case VIR_DOMAIN_NET_TYPE_VHOSTUSER: > @@ -11483,20 +11489,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr > xmlopt, >               goto error; >       } >   -    /* NIC model (see -net nic,model=?).  We only check that it looks > -     * reasonable, not that it is a supported NIC type.  FWIW kvm > -     * supports these types as of April 2008: > -     * i82551 i82557b i82559er ne2k_pci pcnet rtl8139 e1000 virtio > -     * QEMU PPC64 supports spapr-vlan > -     */ > -    if (model != NULL) { > -        if (strspn(model, NET_MODEL_CHARS) < strlen(model)) { > -            virReportError(VIR_ERR_INVALID_ARG, "%s", > -                           _("Model name contains invalid characters")); > -            goto error; > -        } > -        VIR_STEAL_PTR(def->modelstr, model); > -    } > +    if (model != NULL && > +        virDomainNetSetModelString(def, model) < 0) > +        goto error; >         switch (def->type) { >       case VIR_DOMAIN_NET_TYPE_NETWORK: > @@ -21739,6 +21734,14 @@ > virDomainNetDefCheckABIStability(virDomainNetDefPtr src, >           return false; >       } >   +    if (src->model != dst->model) { > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > +                       _("Target network card model %s does not match > source %s"), > +                       virDomainNetModelTypeToString(dst->model), > +                       virDomainNetModelTypeToString(src->model)); > +        return false; > +    } > + >       if (src->mtu != dst->mtu) { >           virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >                          _("Target network card MTU %d does not match > source %d"), > @@ -29379,6 +29382,8 @@ > virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface) >   const char * >   virDomainNetGetModelString(const virDomainNetDef *net) >   { > +    if (net->model) > +        return virDomainNetModelTypeToString(net->model); >       return net->modelstr; >   } >   @@ -29386,13 +29391,31 @@ int >   virDomainNetSetModelString(virDomainNetDefPtr net, >                              const char *model) >   { > -    return VIR_STRDUP(net->modelstr, model); > +    VIR_FREE(net->modelstr); > +    if ((net->model = virDomainNetModelTypeFromString(model)) >= 0) > +        return 0; > + > +    net->model = VIR_DOMAIN_NET_MODEL_UNKNOWN; > +    if (!model) > +        return 0; Is this a safe thing to do? I mean virEnumFromString() (which is called by any TypeFromString() in the end) doesn't handle NULL gracefully. You'll need to swap some lines and probably have a temp variable to store virDomainNetModelTypeFromString() retval, ...