[libvirt] [PATCH 12/28] conf/openvz: eliminate incorrect/undocumented use of <source dev='blah'/>
Laine Stump
laine at laine.org
Fri Jun 24 19:35:42 UTC 2016
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>
>>
More information about the libvir-list
mailing list