2:35 p.m.
On 06/23/2016 06:14 PM, John Ferlan wrote:
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?
Yep. And I had plans to use it again later (see below)
If this was removed, then the RNG would need adjustment as well.
Look up above. dev was removed from <source> for type ethernet (but
<source> was left in the RNG, because I *will* be using that)
> 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;
Nice! I'll do that.
> 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.
When I wrote this patch, I was intending that something new would be
added into the ethernet struct of the union in a followup patch, so I
left all of the *_ETHERNET things in place (and also the ethernet struct
itself). I later changed my mind at the last minute and decided that the
new item (hostIP) should be in the common part of the object rather than
in the ethernet-specific part of the union, so these switch cases and
the struct in the union remain unused for now. I wanted to leave them
that way in case there was any sentiment toward keeping hostIP exclusive
to the ethernet part of the union.
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>
>