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

On 4/16/19 11:27 AM, Michal Privoznik wrote: > 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, ... > Not completely sure I follow, but I think you mean: this function looks like it should operate as virEnumFromString does, meaning return -1 on NULL value. But consider that this is a hybrid enum (net->model) and string (net->modelstr) approach, in which modelstr=NULL is a valid case, so I'm not sure it should be an error.