[libvirt] [PATCH 12/28] conf/openvz: eliminate incorrect/undocumented use of <source dev='blah'/>
Laine Stump
laine at laine.org
Fri Jun 24 20:10:20 UTC 2016
On 06/24/2016 03:35 PM, Laine Stump wrote:
> 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)
Okay, I decided to remove it after all since I don't need it. If someone
needs it in the future they can add it back.
>>
>> 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.
Well, except that I set guest_ifname in a different manner in the else
clause of the if (net->ifname_guest), so it's really more clear if I
leave it as is.
>
>>
>>
>>
>>> 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.
...and now I've decided to merge them all in with the other NOP cases.
>
>
>>
>>
>> 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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list