3:10 p.m.
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list