config: report error when script given for inappropriate interface type

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=638633

Although scripts are not used by interfaces of type other than
"ethernet" in qemu, due to the fact that the parser stores the script
name in a union that is only valid when type is ethernet or bridge,
there is no way for anyone except the parser itself to catch the
problem of specifying an interface script for an inappropriate
interface type (by the time the parsed data gets back to the code that
called the parser, all evidence that a script was specified is
forgotten).

Since the parser itself should be agnostic to which type of interface
allows scripts (an example of why: a script specified for an interface
of type bridge is valid for xen domains, but not for qemu domains),
the solution here is to move the script out of the union(s) in the
DomainNetDef, always populate it when specified (regardless of
interface type), and let the driver decide whether or not it is
appropriate.

Currently the qemu, xen, libxml, and uml drivers recognize the script
parameter and do something with it (the uml driver only to report that
it isn't supported). Those drivers have been updated to log a
CONFIG_UNSUPPORTED error when a script is specified for an interface
type that's inappropriate for that particular hypervisor.

(NB: There was earlier discussion of solving this problem by adding a
VALIDATE flag to all libvirt APIs that accept XML, which would cause
the XML to be validated against the RNG files. One statement during
that discussion was that the RNG shouldn't contain hypervisor-specific
things, though, and a proper solution to this problem would require
that (again, because a script for an interface of type "bridge" is
accepted by xen, but not by qemu).

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 7327667bda83c791db77002b56c46078064935c0..0190a816a3be3d036bc9ce9da27f6cef9a1dc41e 100644 (file)
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -956,7 +956,6 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
     switch (def->type) {
     case VIR_DOMAIN_NET_TYPE_ETHERNET:
         VIR_FREE(def->data.ethernet.dev);
-        VIR_FREE(def->data.ethernet.script);
         VIR_FREE(def->data.ethernet.ipaddr);
         break;
 
@@ -975,7 +974,6 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
 
     case VIR_DOMAIN_NET_TYPE_BRIDGE:
         VIR_FREE(def->data.bridge.brname);
-        VIR_FREE(def->data.bridge.script);
         VIR_FREE(def->data.bridge.ipaddr);
         break;
 
@@ -993,6 +991,7 @@ void virDomainNetDefFree(virDomainNetDefPtr def)
         break;
     }
 
+    VIR_FREE(def->script);
     VIR_FREE(def->ifname);
 
     virDomainDeviceInfoClear(&def->info);
@@ -3764,8 +3763,6 @@ virDomainNetDefParseXML(virCapsPtr caps,
                        xmlStrEqual(cur->name, BAD_CAST "link")) {
                 linkstate = virXMLPropString(cur, "state");
             } else if ((script == NULL) &&
-                       (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET ||
-                        def->type == VIR_DOMAIN_NET_TYPE_BRIDGE) &&
                        xmlStrEqual(cur->name, BAD_CAST "script")) {
                 script = virXMLPropString(cur, "path");
             } else if (xmlStrEqual (cur->name, BAD_CAST "model")) {
@@ -3854,11 +3851,6 @@ virDomainNetDefParseXML(virCapsPtr caps,
         break;
 
     case VIR_DOMAIN_NET_TYPE_ETHERNET:
-
-        if (script != NULL) {
-            def->data.ethernet.script = script;
-            script = NULL;
-        }
         if (dev != NULL) {
             def->data.ethernet.dev = dev;
             dev = NULL;
@@ -3877,10 +3869,6 @@ virDomainNetDefParseXML(virCapsPtr caps,
         }
         def->data.bridge.brname = bridge;
         bridge = NULL;
-        if (script != NULL) {
-            def->data.bridge.script = script;
-            script = NULL;
-        }
         if (address != NULL) {
             def->data.bridge.ipaddr = address;
             address = NULL;
@@ -3957,6 +3945,10 @@ virDomainNetDefParseXML(virCapsPtr caps,
         break;
     }
 
+    if (script != NULL) {
+        def->script = script;
+        script = NULL;
+    }
     if (ifname != NULL) {
         def->ifname = ifname;
         ifname = NULL;
@@ -10340,8 +10332,6 @@ virDomainNetDefFormat(virBufferPtr buf,
         if (def->data.ethernet.ipaddr)
             virBufferAsprintf(buf, "      <ip address='%s'/>\n",
                               def->data.ethernet.ipaddr);
-        virBufferEscapeString(buf, "      <script path='%s'/>\n",
-                              def->data.ethernet.script);
         break;
 
     case VIR_DOMAIN_NET_TYPE_BRIDGE:
@@ -10350,8 +10340,6 @@ virDomainNetDefFormat(virBufferPtr buf,
         if (def->data.bridge.ipaddr)
             virBufferAsprintf(buf, "      <ip address='%s'/>\n",
                               def->data.bridge.ipaddr);
-        virBufferEscapeString(buf, "      <script path='%s'/>\n",
-                              def->data.bridge.script);
         break;
 
     case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -10387,7 +10375,8 @@ virDomainNetDefFormat(virBufferPtr buf,
         break;
     }
 
-
+    virBufferEscapeString(buf, "      <script path='%s'/>\n",
+                          def->script);
     if (def->ifname &&
         !((flags & VIR_DOMAIN_XML_INACTIVE) &&
           (STRPREFIX(def->ifname, VIR_NET_GENERATED_PREFIX)))) {

diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index cd882bb5518c75f17b4841cedf00d5faa1202057..03aa5b69e6626a04f4b993200d60ed1949a18a19 100644 (file)
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -574,7 +574,6 @@ struct _virDomainNetDef {
     union {
         struct {
             char *dev;
-            char *script;
             char *ipaddr;
         } ethernet;
         struct {
@@ -597,7 +596,6 @@ struct _virDomainNetDef {
         } network;
         struct {
             char *brname;
-            char *script;
             char *ipaddr;
         } bridge;
         struct {
@@ -613,6 +611,7 @@ struct _virDomainNetDef {
         bool sndbuf_specified;
         unsigned long sndbuf;
     } tune;
+    char *script;
     char *ifname;
     int bootIndex;
     virDomainDeviceInfo info;

diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index e94e06b5eca551af35b3c3bf1e390870b2a6cead..24830e8e1c9ada07558d663c616b74a3987aec70 100644 (file)
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -618,11 +618,18 @@ libxlMakeNic(virDomainDefPtr def, virDomainNetDefPtr l_nic,
             virReportOOMError();
             return -1;
         }
-        if (l_nic->data.bridge.script &&
-            (x_nic->script = strdup(l_nic->data.bridge.script)) == NULL) {
+        if (l_nic->script &&
+            (x_nic->script = strdup(l_nic->script)) == NULL) {
             virReportOOMError();
             return -1;
         }
+    } else {
+        if (l_nic->script) {
+            libxlError(VIR_ERR_CONFIG_UNSUPPORTED,
+                       _("scripts are not supported on interfaces of type %s"),
+                       virDomainNetTypeToString(l_nic->type));
+            return -1;
+        }
     }
 
     return 0;

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 69bf8681b71be1ec0eaecf83123fde70f040f8a6..f2e9cfab463b015ca5434a7bc75249e4f7623218 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -2420,8 +2420,16 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
 {
     bool is_tap = false;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
+    enum virDomainNetType netType = virDomainNetGetActualType(net);
 
-    switch (virDomainNetGetActualType(net)) {
+    if (net->script && netType != VIR_DOMAIN_NET_TYPE_ETHERNET) {
+        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                        _("scripts are not supported on interfaces of type %s"),
+                        virDomainNetTypeToString(netType));
+        return NULL;
+    }
+
+    switch (netType) {
     case VIR_DOMAIN_NET_TYPE_NETWORK:
     case VIR_DOMAIN_NET_TYPE_BRIDGE:
     case VIR_DOMAIN_NET_TYPE_DIRECT:
@@ -2437,9 +2445,9 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
             virBufferAsprintf(&buf, "%cifname=%s", type_sep, net->ifname);
             type_sep = ',';
         }
-        if (net->data.ethernet.script) {
+        if (net->script) {
             virBufferAsprintf(&buf, "%cscript=%s", type_sep,
-                              net->data.ethernet.script);
+                              net->script);
             type_sep = ',';
         }
         is_tap = true;
@@ -2449,7 +2457,7 @@ qemuBuildHostNetStr(virDomainNetDefPtr net,
     case VIR_DOMAIN_NET_TYPE_SERVER:
     case VIR_DOMAIN_NET_TYPE_MCAST:
         virBufferAddLit(&buf, "socket");
-        switch (virDomainNetGetActualType(net)) {
+        switch (netType) {
         case VIR_DOMAIN_NET_TYPE_CLIENT:
             virBufferAsprintf(&buf, "%cconnect=%s:%d",
                               type_sep,
@@ -6255,7 +6263,7 @@ qemuParseCommandLineNet(virCapsPtr caps,
             }
         } else if (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
                    STREQ(keywords[i], "script") && STRNEQ(values[i], "")) {
-            def->data.ethernet.script = values[i];
+            def->script = values[i];
             values[i] = NULL;
         } else if (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
                    STREQ(keywords[i], "ifname")) {

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c5b08f95528c2fbe3a37a356a4860a417043e451..6b03c6238ab4eda6c70d46ef7284b2c32620f278 100644 (file)
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -1167,10 +1167,12 @@ void qemuDomainObjCheckNetTaint(struct qemud_driver *driver,
                                 virDomainNetDefPtr net,
                                 int logFD)
 {
-    if ((net->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
-         net->data.ethernet.script != NULL) ||
-        (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE &&
-         net->data.bridge.script != NULL))
+    /* script is only useful for NET_TYPE_ETHERNET (qemu) and
+     * NET_TYPE_BRIDGE (xen), but could be (incorrectly) specified for
+     * any interface type. In any case, it's adding user sauce into
+     * the soup, so it should taint the domain.
+     */
+    if (net->script != NULL)
         qemuDomainObjTaint(driver, obj, VIR_DOMAIN_TAINT_SHELL_SCRIPTS, logFD);
 }
 

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b60aad67e5c4059e04a16310c5b0914af37fab45..2c4d071792267ab6365914a1d56b99650f6bf50c 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4470,8 +4470,8 @@ static char *qemuDomainXMLToNative(virConnectPtr conn,
                 memset(net, 0, sizeof *net);
 
                 net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
+                net->script = NULL;
                 net->data.ethernet.dev = brnamecopy;
-                net->data.ethernet.script = NULL;
                 net->data.ethernet.ipaddr = NULL;
             } else {
                 /* actualType is either NETWORK or DIRECT. In either
@@ -4481,8 +4481,8 @@ static char *qemuDomainXMLToNative(virConnectPtr conn,
                 memset(net, 0, sizeof *net);
 
                 net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
+                net->script = NULL;
                 net->data.ethernet.dev = NULL;
-                net->data.ethernet.script = NULL;
                 net->data.ethernet.ipaddr = NULL;
             }
         } else if (net->type == VIR_DOMAIN_NET_TYPE_DIRECT) {
@@ -4492,19 +4492,19 @@ static char *qemuDomainXMLToNative(virConnectPtr conn,
             memset(net, 0, sizeof *net);
 
             net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
+            net->script = NULL;
             net->data.ethernet.dev = NULL;
-            net->data.ethernet.script = NULL;
             net->data.ethernet.ipaddr = NULL;
         } else if (net->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
+            char *script = net->script;
             char *brname = net->data.bridge.brname;
-            char *script = net->data.bridge.script;
             char *ipaddr = net->data.bridge.ipaddr;
 
             memset(net, 0, sizeof *net);
 
             net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
+            net->script = script;
             net->data.ethernet.dev = brname;
-            net->data.ethernet.script = script;
             net->data.ethernet.ipaddr = ipaddr;
         }
         net->bootIndex = bootIndex;

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
index f3597a15679f6c4cf789c116b279df8faff06a5c..99f53d5fbe7e9d3560058904d3884b658ebdc2b6 100644 (file)
--- a/src/qemu/qemu_hotplug.c
+++ b/src/qemu/qemu_hotplug.c
@@ -1219,7 +1219,7 @@ int qemuDomainChangeNet(struct qemud_driver *driver,
 
     case VIR_DOMAIN_NET_TYPE_ETHERNET:
         if (STRNEQ_NULLABLE(olddev->data.ethernet.dev, dev->data.ethernet.dev) ||
-            STRNEQ_NULLABLE(olddev->data.ethernet.script, dev->data.ethernet.script) ||
+            STRNEQ_NULLABLE(olddev->script, dev->script) ||
             STRNEQ_NULLABLE(olddev->data.ethernet.ipaddr, dev->data.ethernet.ipaddr)) {
             qemuReportError(VIR_ERR_NO_SUPPORT,
                             _("cannot modify ethernet network device configuration"));

diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c
index 86ca191de00ab4185f1c93959e8f746dbe8b3d4f..316d558897d0f7f082713850a8086e65990b879c 100644 (file)
--- a/src/uml/uml_conf.c
+++ b/src/uml/uml_conf.c
@@ -193,11 +193,6 @@ umlBuildCommandLineNet(virConnectPtr conn,
                            _("IP address not supported for ethernet interface"));
             goto error;
         }
-        if (def->data.ethernet.script) {
-            umlReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-                           _("script execution not supported for ethernet interface"));
-            goto error;
-        }
         break;
 
     case VIR_DOMAIN_NET_TYPE_SERVER:
@@ -265,6 +260,12 @@ umlBuildCommandLineNet(virConnectPtr conn,
         break;
     }
 
+    if (def->script) {
+        umlReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("interface script execution not supported by this driver"));
+        goto error;
+    }
+
     virBufferAsprintf(&buf, ",%02x:%02x:%02x:%02x:%02x:%02x",
                       def->mac[0], def->mac[1], def->mac[2],
                       def->mac[3], def->mac[4], def->mac[5]);

diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c
index 9909d13af3dc0b42eee9ecb3c360a44490f470d6..b3267a901002d5d7ecb9be24e09ce8d0c83b54e9 100644 (file)
--- a/src/vbox/vbox_tmpl.c
+++ b/src/vbox/vbox_tmpl.c
@@ -4364,7 +4364,7 @@ vboxAttachNetwork(virDomainDefPtr def, vboxGlobalData *data, IMachine *machine)
             VIR_DEBUG("NIC(%d): NAT.", i);
         } else if (def->nets[i]->type == VIR_DOMAIN_NET_TYPE_BRIDGE) {
             VIR_DEBUG("NIC(%d): brname: %s", i, def->nets[i]->data.bridge.brname);
-            VIR_DEBUG("NIC(%d): script: %s", i, def->nets[i]->data.bridge.script);
+            VIR_DEBUG("NIC(%d): script: %s", i, def->nets[i]->script);
             VIR_DEBUG("NIC(%d): ipaddr: %s", i, def->nets[i]->data.bridge.ipaddr);
         }
 

diff --git a/src/xenxs/xen_sxpr.c b/src/xenxs/xen_sxpr.c
index 124c36978633b07be94a74dd44303fd4e54f2812..04ba5aaa4d027619935ab50b955f3a541b533fb5 100644 (file)
--- a/src/xenxs/xen_sxpr.c
+++ b/src/xenxs/xen_sxpr.c
@@ -549,7 +549,7 @@ xenParseSxprNets(virDomainDefPtr def,
                     goto no_memory;
                 if (tmp2 &&
                     net->type == VIR_DOMAIN_NET_TYPE_BRIDGE &&
-                    !(net->data.bridge.script = strdup(tmp2)))
+                    !(net->script = strdup(tmp2)))
                     goto no_memory;
                 tmp = sexpr_node(node, "device/vif/ip");
                 if (tmp &&
@@ -558,7 +558,7 @@ xenParseSxprNets(virDomainDefPtr def,
             } else {
                 net->type = VIR_DOMAIN_NET_TYPE_ETHERNET;
                 if (tmp2 &&
-                    !(net->data.ethernet.script = strdup(tmp2)))
+                    !(net->script = strdup(tmp2)))
                     goto no_memory;
                 tmp = sexpr_node(node, "device/vif/ip");
                 if (tmp &&
@@ -1786,6 +1786,14 @@ xenFormatSxprNet(virConnectPtr conn,
                      _("unsupported network type %d"), def->type);
         return -1;
     }
+    if (def->script &&
+        def->type != VIR_DOMAIN_NET_TYPE_BRIDGE &&
+        def->type != VIR_DOMAIN_NET_TYPE_ETHERNET) {
+        XENXS_ERROR(VIR_ERR_CONFIG_UNSUPPORTED,
+                    _("scripts are not supported on interfaces of type %s"),
+                    virDomainNetTypeToString(def->type));
+        return -1;
+    }
 
     if (!isAttach)
         virBufferAddLit(buf, "(device ");
@@ -1800,8 +1808,8 @@ xenFormatSxprNet(virConnectPtr conn,
     switch (def->type) {
     case VIR_DOMAIN_NET_TYPE_BRIDGE:
         virBufferEscapeSexpr(buf, "(bridge '%s')", def->data.bridge.brname);
-        if (def->data.bridge.script)
-            script = def->data.bridge.script;
+        if (def->script)
+            script = def->script;
 
         virBufferEscapeSexpr(buf, "(script '%s')", script);
         if (def->data.bridge.ipaddr != NULL)
@@ -1835,9 +1843,9 @@ xenFormatSxprNet(virConnectPtr conn,
     break;
 
     case VIR_DOMAIN_NET_TYPE_ETHERNET:
-        if (def->data.ethernet.script)
+        if (def->script)
             virBufferEscapeSexpr(buf, "(script '%s')",
-                                 def->data.ethernet.script);
+                                 def->script);
         if (def->data.ethernet.ipaddr != NULL)
             virBufferEscapeSexpr(buf, "(ip '%s')", def->data.ethernet.ipaddr);
         break;

diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c
index b415ce8c33abd8a302615203855f23d9e277db8b..0aa04f3128f833ecbd32a7cb7ea71ef28edc214a 100644 (file)
--- a/src/xenxs/xen_xm.c
+++ b/src/xenxs/xen_xm.c
@@ -708,21 +708,19 @@ xenParseXM(virConfPtr conf, int xendConfigVersion,
                 if (bridge[0] &&
                     !(net->data.bridge.brname = strdup(bridge)))
                     goto no_memory;
-                if (script[0] &&
-                    !(net->data.bridge.script = strdup(script)))
-                    goto no_memory;
                 if (ip[0] &&
                     !(net->data.bridge.ipaddr = strdup(ip)))
                     goto no_memory;
             } else {
-                if (script && script[0] &&
-                    !(net->data.ethernet.script = strdup(script)))
-                    goto no_memory;
                 if (ip[0] &&
                     !(net->data.ethernet.ipaddr = strdup(ip)))
                     goto no_memory;
             }
 
+            if (script && script[0] &&
+                !(net->script = strdup(script)))
+               goto no_memory;
+
             if (model[0] &&
                 !(net->model = strdup(model)))
                 goto no_memory;
@@ -1282,8 +1280,8 @@ static int xenFormatXMNet(virConnectPtr conn,
         break;
 
     case VIR_DOMAIN_NET_TYPE_ETHERNET:
-        if (net->data.ethernet.script)
-            virBufferAsprintf(&buf, ",script=%s", net->data.ethernet.script);
+        if (net->script)
+            virBufferAsprintf(&buf, ",script=%s", net->script);
         if (net->data.ethernet.ipaddr)
             virBufferAsprintf(&buf, ",ip=%s", net->data.ethernet.ipaddr);
         break;