diff mbox series

[04/18] conf: Add virDomainNetHasVirtioModel

Message ID f7fbd203b37ecdc5292167026d615ba6b0a9fa45.1547746867.git.crobinso@redhat.com
State New
Headers show
Series qemu: virtio-{non-}transitional support | expand

Commit Message

Cole Robinson Jan. 17, 2019, 5:52 p.m. UTC
This will be extended in the future, so let's simplify things by
centralizing the checks.

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/conf/domain_conf.c         | 47 +++++++++++++++++++---------------
 src/conf/domain_conf.h         |  1 +
 src/libvirt_private.syms       |  1 +
 src/qemu/qemu_domain.c         |  2 +-
 src/qemu/qemu_domain_address.c |  3 +--
 src/qemu/qemu_hotplug.c        |  2 +-
 src/qemu/qemu_interface.c      |  8 +++---
 src/security/virt-aa-helper.c  |  2 +-
 tests/qemuxml2argvmock.c       |  2 +-
 9 files changed, 38 insertions(+), 30 deletions(-)

-- 
2.20.1

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

Comments

Andrea Bolognani Jan. 18, 2019, 1:35 p.m. UTC | #1
On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:
[...]
> @@ -11329,6 +11329,22 @@ 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;

> +        }

> +        def->model = model;

> +        model = NULL;

> +    }


Can you please split this code motion...

> +

>      switch (def->type) {

>      case VIR_DOMAIN_NET_TYPE_NETWORK:

>          if (network == NULL) {

> @@ -11346,7 +11362,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,

>          break;

>  

>      case VIR_DOMAIN_NET_TYPE_VHOSTUSER:

> -        if (STRNEQ_NULLABLE(model, "virtio")) {

> +        if (!virDomainNetHasVirtioModel(def)) {

>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

>                             _("Wrong or no <model> 'type' attribute "

>                               "specified with <interface type='vhostuser'/>. "


... along with adjusting this from model to def->model, off to its
own preparatory patch?

[...]
> +bool

> +virDomainNetHasVirtioModel(const virDomainNetDef *iface)

> +{

> +    return STREQ_NULLABLE(iface->model, "virtio");

> +}


I'd probably call this virDomainNetIsModelVirtio() and call the
argument 'net', but your version is fine too if you prefer it.

With the preparatory work in a separate patch,

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>


-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson Jan. 21, 2019, 10:43 p.m. UTC | #2
On 01/18/2019 08:35 AM, Andrea Bolognani wrote:
> On Thu, 2019-01-17 at 12:52 -0500, Cole Robinson wrote:

> [...]

>> @@ -11329,6 +11329,22 @@ 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;

>> +        }

>> +        def->model = model;

>> +        model = NULL;

>> +    }

> 

> Can you please split this code motion...

> 

>> +

>>      switch (def->type) {

>>      case VIR_DOMAIN_NET_TYPE_NETWORK:

>>          if (network == NULL) {

>> @@ -11346,7 +11362,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,

>>          break;

>>  

>>      case VIR_DOMAIN_NET_TYPE_VHOSTUSER:

>> -        if (STRNEQ_NULLABLE(model, "virtio")) {

>> +        if (!virDomainNetHasVirtioModel(def)) {

>>              virReportError(VIR_ERR_INTERNAL_ERROR, "%s",

>>                             _("Wrong or no <model> 'type' attribute "

>>                               "specified with <interface type='vhostuser'/>. "

> 

> ... along with adjusting this from model to def->model, off to its

> own preparatory patch?

> 

> [...]

>> +bool

>> +virDomainNetHasVirtioModel(const virDomainNetDef *iface)

>> +{

>> +    return STREQ_NULLABLE(iface->model, "virtio");

>> +}

> 

> I'd probably call this virDomainNetIsModelVirtio() and call the

> argument 'net', but your version is fine too if you prefer it.

> 

> With the preparatory work in a separate patch,

> 

>   Reviewed-by: Andrea Bolognani <abologna@redhat.com>

> 


I'll adjust it all. The iface naming was following some similar
functions above it in domain_conf.c but it's certainly less common than
'net' naming

Thanks,
Cole

- Cole

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

Patch

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index facb84a425..4fc33c7e2f 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -4737,7 +4737,7 @@  virDomainDeviceDefPostParseCommon(virDomainDeviceDefPtr dev,
 
     if (dev->type == VIR_DOMAIN_DEVICE_NET) {
         virDomainNetDefPtr net = dev->data.net;
-        if (STRNEQ_NULLABLE(net->model, "virtio") &&
+        if (!virDomainNetHasVirtioModel(net) &&
             virDomainCheckVirtioOptions(net->virtio) < 0)
             return -1;
     }
@@ -11329,6 +11329,22 @@  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;
+        }
+        def->model = model;
+        model = NULL;
+    }
+
     switch (def->type) {
     case VIR_DOMAIN_NET_TYPE_NETWORK:
         if (network == NULL) {
@@ -11346,7 +11362,7 @@  virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
         break;
 
     case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
-        if (STRNEQ_NULLABLE(model, "virtio")) {
+        if (!virDomainNetHasVirtioModel(def)) {
             virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                            _("Wrong or no <model> 'type' attribute "
                              "specified with <interface type='vhostuser'/>. "
@@ -11574,24 +11590,8 @@  virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
         ifname_guest_actual = NULL;
     }
 
-    /* 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;
-        }
-        def->model = model;
-        model = NULL;
-    }
-
     if (def->type != VIR_DOMAIN_NET_TYPE_HOSTDEV &&
-        STREQ_NULLABLE(def->model, "virtio")) {
+        virDomainNetHasVirtioModel(def)) {
         if (backend != NULL) {
             if ((val = virDomainNetBackendTypeFromString(backend)) < 0 ||
                 val == VIR_DOMAIN_NET_BACKEND_TYPE_DEFAULT) {
@@ -25478,7 +25478,7 @@  virDomainNetDefFormat(virBufferPtr buf,
     if (def->model) {
         virBufferEscapeString(buf, "<model type='%s'/>\n",
                               def->model);
-        if (STREQ(def->model, "virtio")) {
+        if (virDomainNetHasVirtioModel(def)) {
             char *str = NULL, *gueststr = NULL, *hoststr = NULL;
             int rc = 0;
 
@@ -29732,6 +29732,13 @@  virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface)
 }
 
 
+bool
+virDomainNetHasVirtioModel(const virDomainNetDef *iface)
+{
+    return STREQ_NULLABLE(iface->model, "virtio");
+}
+
+
 /* Return listens[i] from the appropriate union for the graphics
  * type, or NULL if this is an unsuitable type, or the index is out of
  * bounds. If force0 is TRUE, i == 0, and there is no listen array,
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 16c2c6e75b..e3f4273b55 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -3222,6 +3222,7 @@  virNetDevBandwidthPtr
 virDomainNetGetActualBandwidth(virDomainNetDefPtr iface);
 virNetDevVlanPtr virDomainNetGetActualVlan(virDomainNetDefPtr iface);
 bool virDomainNetGetActualTrustGuestRxFilters(virDomainNetDefPtr iface);
+bool virDomainNetHasVirtioModel(const virDomainNetDef *iface);
 int virDomainNetAppendIPAddress(virDomainNetDefPtr def,
                                 const char *address,
                                 int family,
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0d6f054cf2..bd7e896654 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -470,6 +470,7 @@  virDomainNetGetActualTrustGuestRxFilters;
 virDomainNetGetActualType;
 virDomainNetGetActualVirtPortProfile;
 virDomainNetGetActualVlan;
+virDomainNetHasVirtioModel;
 virDomainNetInsert;
 virDomainNetNotifyActualDevice;
 virDomainNetReleaseActualDevice;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ec6b340308..08bb2f9ebc 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -4573,7 +4573,7 @@  qemuDomainDeviceDefValidateNetwork(const virDomainNetDef *net)
         return -1;
     }
 
-    if (STREQ_NULLABLE(net->model, "virtio")) {
+    if (virDomainNetHasVirtioModel(net)) {
         if (net->driver.virtio.rx_queue_size & (net->driver.virtio.rx_queue_size - 1)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                            _("rx_queue_size has to be a power of two"));
diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c
index 4a7c71d76d..0950197a44 100644
--- a/src/qemu/qemu_domain_address.c
+++ b/src/qemu/qemu_domain_address.c
@@ -322,8 +322,7 @@  qemuDomainPrimeVirtioDeviceAddresses(virDomainDefPtr def,
     for (i = 0; i < def->nnets; i++) {
         virDomainNetDefPtr net = def->nets[i];
 
-        if (net->model &&
-            STREQ(net->model, "virtio") &&
+        if (virDomainNetHasVirtioModel(net) &&
             net->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) {
             net->info.type = type;
         }
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index a1c3ca999b..2fdc71d07b 100644
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -3710,7 +3710,7 @@  qemuDomainChangeNet(virQEMUDriverPtr driver,
         goto cleanup;
     }
 
-    if (olddev->model && STREQ(olddev->model, "virtio") &&
+    if (virDomainNetHasVirtioModel(olddev) &&
         (olddev->driver.virtio.name != newdev->driver.virtio.name ||
          olddev->driver.virtio.txmode != newdev->driver.virtio.txmode ||
          olddev->driver.virtio.ioeventfd != newdev->driver.virtio.ioeventfd ||
diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c
index 2607dea1f5..2cbb382b10 100644
--- a/src/qemu/qemu_interface.c
+++ b/src/qemu/qemu_interface.c
@@ -261,7 +261,7 @@  qemuInterfaceDirectConnect(virDomainDefPtr def,
     virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
     unsigned int macvlan_create_flags = VIR_NETDEV_MACVLAN_CREATE_WITH_TAP;
 
-    if (net->model && STREQ(net->model, "virtio"))
+    if (virDomainNetHasVirtioModel(net))
         macvlan_create_flags |= VIR_NETDEV_MACVLAN_VNET_HDR;
 
     if (virNetDevMacVLanCreateWithVPortProfile(net->ifname,
@@ -434,7 +434,7 @@  qemuInterfaceEthernetConnect(virDomainDefPtr def,
         template_ifname = true;
     }
 
-    if (net->model && STREQ(net->model, "virtio"))
+    if (virDomainNetHasVirtioModel(net))
         tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
 
     if (virNetDevTapCreate(&net->ifname, tunpath, tapfd, tapfdSize,
@@ -533,7 +533,7 @@  qemuInterfaceBridgeConnect(virDomainDefPtr def,
         template_ifname = true;
     }
 
-    if (net->model && STREQ(net->model, "virtio"))
+    if (virDomainNetHasVirtioModel(net))
         tap_create_flags |= VIR_NETDEV_TAP_CREATE_VNET_HDR;
 
     if (virQEMUDriverIsPrivileged(driver)) {
@@ -653,7 +653,7 @@  qemuInterfaceOpenVhostNet(virDomainDefPtr def,
     }
 
     /* If the nic model isn't virtio, don't try to open. */
-    if (!(net->model && STREQ(net->model, "virtio"))) {
+    if (!virDomainNetHasVirtioModel(net)) {
         if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_VHOST) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                            "%s", _("vhost-net is only supported for "
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 64a425671d..3bd30bb417 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -1241,7 +1241,7 @@  get_files(vahControl * ctl)
             if (net && net->model) {
                 if (net->driver.virtio.name == VIR_DOMAIN_NET_BACKEND_TYPE_QEMU)
                     continue;
-                if (STRNEQ(net->model, "virtio"))
+                if (!virDomainNetHasVirtioModel(net))
                     continue;
             }
             needsvhost = true;
diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
index b300793732..729e8146b1 100644
--- a/tests/qemuxml2argvmock.c
+++ b/tests/qemuxml2argvmock.c
@@ -228,7 +228,7 @@  qemuInterfaceOpenVhostNet(virDomainDefPtr def ATTRIBUTE_UNUSED,
 {
     size_t i;
 
-    if (!(net->model && STREQ(net->model, "virtio"))) {
+    if (!virDomainNetHasVirtioModel(net)) {
         *vhostfdSize = 0;
         return 0;
     }