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