Re: [PATCH 12/28] conf/openvz: eliminate incorrect/undocumented use of <source dev='blah'/>

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 06/22/2016 01:37 PM, Laine Stump wrote:
> When support for <interface type='ethernet'> was added in commit
> 9a4b705f back in 2010, it erroneously looked at <source dev='blah'/>
> for a user-specified guest-side interface name. This was never
> documented though. (that attribute already existed at the time in the
> data.ethernet union member of virDomainNetDef, but apparently had no
> practical use - it was only used as a storage place for a NetDef's
> bridge name during qemuDomainXMLToNative(), but even then that was
> never used for anything).
> 
> When support for similar guest-side device naming was added to the lxc
> driver several years later, it was put in a new subelement <guest
> dev='blah'/>.
> 
> In the intervening years, since there was no validation that
> ethernet.dev was NULL in the other drivers that didn't actually use
> it, innocent souls who were adding other features assuming they needed
> to account for non-NULL ethernet.dev when really they didn't, so
> little bits of the usual pointless cargo-cult code showed up.
> 
> This patch not only switches the openvz driver to use the documented
> <guest dev='blah'/> notation for naming the guest-side device (just in
> case anyone is still using the openvz driver), and logs an error if
> anyone tries to set <source dev='blah'/> for a type='ethernet'
> interface, it also removes the cargo-cult uses of ethernet.dev and
> <source dev='blah'/>, and eliminates if from the RNG and from
> virDomainNetDef.
> 
> NB: I decided on this course of action after mentioning the
> inconsistency here:
> 
>   https://www.redhat.com/archives/libvir-list/2016-May/msg02038.html
> 
> and getting encouragement do eliminate it in a later IRC discussion
> with danpb.
> ---
>  docs/schemas/domaincommon.rng                |  3 ---
>  src/conf/domain_conf.c                       | 32 +++++++++++++++++++---------
>  src/conf/domain_conf.h                       |  1 -
>  src/openvz/openvz_driver.c                   |  5 ++---
>  src/qemu/qemu_hotplug.c                      |  6 +-----
>  tests/xml2sexprdata/xml2sexpr-net-routed.xml |  1 -
>  6 files changed, 25 insertions(+), 23 deletions(-)
> 

I'll be impressed if someone finds your needle-in-the-haystack message
in virDomainNetDefParseXML regarding openvz driver and deprecation.  My
only words of wisdom there are - could it cause a guest to disappear now
that previously was visible? I'm all for keeping it as written here
though, but there could be someone else needing some TUMS.


> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 162c2e0..b81b558 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2142,9 +2142,6 @@
>            <interleave>
>              <optional>
>                <element name="source">
> -                <attribute name="dev">
> -                  <ref name="deviceName"/>
> -                </attribute>
>                  <empty/>
>                </element>
>              </optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 899b6af..4802e03 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1749,7 +1749,6 @@ virDomainNetDefClear(virDomainNetDefPtr def)
>  
>      switch (def->type) {
>      case VIR_DOMAIN_NET_TYPE_ETHERNET:
> -        VIR_FREE(def->data.ethernet.dev);
>          break;
>  
>      case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> @@ -9004,12 +9003,31 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>                         def->type == VIR_DOMAIN_NET_TYPE_BRIDGE &&
>                         xmlStrEqual(cur->name, BAD_CAST "source")) {
>                  bridge = virXMLPropString(cur, "bridge");
> -            } else if (!dev &&
> -                       (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET ||
> -                        def->type == VIR_DOMAIN_NET_TYPE_DIRECT) &&
> +            } else if (!dev && def->type == VIR_DOMAIN_NET_TYPE_DIRECT &&
>                         xmlStrEqual(cur->name, BAD_CAST "source")) {
>                  dev  = virXMLPropString(cur, "dev");
>                  mode = virXMLPropString(cur, "mode");
> +            } else if (!dev && def->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
> +                       xmlStrEqual(cur->name, BAD_CAST "source")) {
> +                /* This clause is only necessary because from 2010 to
> +                 * 2016 it was possible (but never documented) to
> +                 * configure the name of the guest-side interface of
> +                 * an openvz domain with <source dev='blah'/>.  That
> +                 * was blatant misuse of <source>, so was likely
> +                 * (hopefully) never used, but just in case there was
> +                 * somebody using it, we need to generate an error. If
> +                 * the openvz driver is ever deprecated, this clause
> +                 * can be removed from here.
> +                 */
> +                if ((dev = virXMLPropString(cur, "dev"))) {
> +                    virReportError(VIR_ERR_XML_ERROR,
> +                                   _("Invalid attempt to set <interface type='ethernet'> "
> +                                     "device name with <source dev='%s'/>. "
> +                                     "Use <target dev='%s'/> (for host-side) "
> +                                     "or <guest dev='%s'/> (for guest-side) instead."),
> +                                   dev, dev, dev);
> +                    goto error;
> +                }
>              } else if (!vhostuser_path && !vhostuser_mode && !vhostuser_type
>                         && def->type == VIR_DOMAIN_NET_TYPE_VHOSTUSER &&
>                         xmlStrEqual(cur->name, BAD_CAST "source")) {
> @@ -9260,10 +9278,6 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt,
>          break;
>  
>      case VIR_DOMAIN_NET_TYPE_ETHERNET:
> -        if (dev != NULL) {
> -            def->data.ethernet.dev = dev;
> -            dev = NULL;
> -        }
>          break;
>  
>      case VIR_DOMAIN_NET_TYPE_BRIDGE:
> @@ -20787,8 +20801,6 @@ virDomainNetDefFormat(virBufferPtr buf,
>              break;
>  
>          case VIR_DOMAIN_NET_TYPE_ETHERNET:
> -            virBufferEscapeString(buf, "<source dev='%s'/>\n",
> -                                  def->data.ethernet.dev);
>              break;
>  
>          case VIR_DOMAIN_NET_TYPE_VHOSTUSER:
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index b9dc174..e93bd5c 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -931,7 +931,6 @@ struct _virDomainNetDef {
>      } backend;
>      union {
>          struct {
> -            char *dev;
>          } ethernet;

So an empty ethernet struct is OK?

If this was removed, then the RNG would need adjustment as well.

>          virDomainChrSourceDefPtr vhostuser;
>          struct {
> diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c
> index b66883f..b114246 100644
> --- a/src/openvz/openvz_driver.c
> +++ b/src/openvz/openvz_driver.c
> @@ -862,9 +862,8 @@ openvzDomainSetNetwork(virConnectPtr conn, const char *vpsid,
>  
>          /* if net is ethernet and the user has specified guest interface name,
>           * let's use it; otherwise generate a new one */
> -        if (net->type == VIR_DOMAIN_NET_TYPE_ETHERNET &&
> -            net->data.ethernet.dev != NULL) {
> -            if (VIR_STRDUP(guest_ifname, net->data.ethernet.dev) == -1)
> +        if (net->ifname_guest) {
> +            if (VIR_STRDUP(guest_ifname, net->ifname_guest) < 0)

I believe VIR_STRDUP does the right thing, no need for the "if ()"

virStrdup()

   *dest = NULL;
   if (!src)
       return 0;



>                  goto cleanup;
>          } else {
>              guest_ifname = openvzGenerateContainerVethName(veid);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index f695903..e0b8230 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -2345,11 +2345,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver,
>              break;
>  
>          case VIR_DOMAIN_NET_TYPE_ETHERNET:

You could move this up with the VIR_DOMAIN_NET_TYPE_USER since both just
break;  - your call on that.


ACK in principle... Although I don't think you need that ethernet struct
any more.


John
> -            if (STRNEQ_NULLABLE(olddev->data.ethernet.dev,
> -                                newdev->data.ethernet.dev)) {
> -                needReconnect = true;
> -            }
> -        break;
> +            break;
>  
>          case VIR_DOMAIN_NET_TYPE_SERVER:
>          case VIR_DOMAIN_NET_TYPE_CLIENT:
> diff --git a/tests/xml2sexprdata/xml2sexpr-net-routed.xml b/tests/xml2sexprdata/xml2sexpr-net-routed.xml
> index f34dbaa..2adc3a7 100644
> --- a/tests/xml2sexprdata/xml2sexpr-net-routed.xml
> +++ b/tests/xml2sexprdata/xml2sexpr-net-routed.xml
> @@ -20,7 +20,6 @@
>      <interface type="ethernet">
>        <mac address="00:11:22:33:44:55"/>
>        <ip address="172.14.5.6"/>
> -      <source dev="eth3"/>
>        <script path="vif-routed"/>
>        <target dev="vif4.0"/>
>      </interface>
> 

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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]