On 4/16/19 7:40 PM, Cole Robinson wrote:
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 <crobi...@redhat.com>
---
   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.

I know this is a hybrid, but calling virDomainNetSetModelString(net, NULL) will lead to instant crash. Because model(=NULL) is passed to virDomainNetModeTypeFromString() which dereferences it, and only after that there's the check if (!model). True, in 08/11 you're fixing this so not big of a deal, but if somebody wants to cherry-pick this one they also need to back port 08/11.

Michal

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

Reply via email to