[libvirt] [PATCH] allow to control host side link status of ethernet network device
Back to old thread with Laine Stump with message title: "qemu: remove unnecessary setting of tap device online state" I'm not tested ip and route assign in case of up/down link and device update on the fly. But host side link status tested and worked fine. Vasiliy Tolstov (1): qemu: allow to control host side link status of network device docs/formatdomain.html.in | 21 +++++++++++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_hotplug.c | 17 +++++++++++++++++ src/qemu/qemu_interface.c | 8 ++++---- 6 files changed, 82 insertions(+), 4 deletions(-) -- 2.9.3
Signed-off-by: Vasiliy Tolstov <v.tolstov@selfip.ru> --- docs/formatdomain.html.in | 21 +++++++++++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_hotplug.c | 17 +++++++++++++++++ src/qemu/qemu_interface.c | 8 ++++---- 6 files changed, 82 insertions(+), 4 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8c884f4af9cb..dd8e6a4afa99 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5421,6 +5421,27 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.9.5</span> </p> + <h5><a name="elementHostLink">Modifying phisical link state</a></h5> +<pre> +... +<devices> + <interface type='ethernet'> + <source> + <b><link state='down'/></b> + <target dev='vnet0'/> + </interface> +</devices> +...</pre> + + <p> + This element provides means of setting state of the phisical network interface. + Possible values for attribute <code>state</code> are <code>up</code> and + <code>down</code>. If <code>down</code> is specified as the value, the interface + put in down state. Default behavior if this element is unspecified is to have the + link state <code>up</code>. + <span class="since">Since 3.3.2</span> + </p> + <h5><a name="mtu">MTU configuration</a></h5> <pre> ... diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 281309ec09da..89213d63b6e9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2798,6 +2798,17 @@ All ip-related info for either the host or guest side of an interface --> <define name="interface-ip-info"> + <optional> + <element name="link"> + <attribute name="state"> + <choice> + <value>up</value> + <value>down</value> + </choice> + </attribute> + <empty/> + </element> + </optional> <zeroOrMore> <element name="ip"> <attribute name="address"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0ff216e3a373..b7398276af57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9606,6 +9606,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *devaddr = NULL; char *mode = NULL; char *linkstate = NULL; + char *hostlinkstate = NULL; char *addrtype = NULL; char *domain_name = NULL; char *vhostuser_mode = NULL; @@ -9654,6 +9655,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (virDomainNetIPInfoParseXML(_("interface host IP"), ctxt, &def->hostIP) < 0) goto error; + + if (!hostlinkstate) + hostlinkstate = virXPathString("string(./link/@state)", ctxt); + ctxt->node = tmpnode; } if (!macaddr && xmlStrEqual(cur->name, BAD_CAST "mac")) { @@ -10303,6 +10308,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } } + def->hostlinkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT; + if (hostlinkstate != NULL) { + if ((def->hostlinkstate = virDomainNetInterfaceLinkStateTypeFromString(hostlinkstate)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown interface link state '%s'"), + hostlinkstate); + goto error; + } + } + if (filter != NULL) { switch (def->type) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -10371,6 +10386,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(devaddr); VIR_FREE(mode); VIR_FREE(linkstate); + VIR_FREE(hostlinkstate); VIR_FREE(addrtype); VIR_FREE(domain_name); VIR_FREE(trustGuestRxFilters); @@ -22113,6 +22129,18 @@ virDomainNetDefFormat(virBufferPtr buf, break; } + if (def->hostlinkstate) { + if (sourceLines == 0) { + virBufferAddLit(buf, "<source>\n"); + sourceLines += 2; + } + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<link state='%s'/>\n", + virDomainNetInterfaceLinkStateTypeToString(def->hostlinkstate)); + virBufferAdjustIndent(buf, -2); + sourceLines += 2; + } + /* if sourceLines == 0 - no <source> info at all so far * sourceLines == 1 - first line written, no terminating ">" * sourceLines > 1 - multiple lines, including subelements diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 09fb7aada4b2..71e12a30c2c1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1037,6 +1037,7 @@ struct _virDomainNetDef { virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ int linkstate; + int hostlinkstate; unsigned int mtu; virNetDevCoalescePtr coalesce; }; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e8d29186eb32..7fc41b28d9f8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2978,6 +2978,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, bool needBridgeChange = false; bool needFilterChange = false; bool needLinkStateChange = false; + bool needHostLinkStateChange = false; bool needReplaceDevDef = false; bool needBandwidthSet = false; int ret = -1; @@ -3264,6 +3265,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, if (olddev->linkstate != newdev->linkstate) needLinkStateChange = true; + if (olddev->hostlinkstate != newdev->hostlinkstate) + needHostLinkStateChange = true; + if (!virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev), virDomainNetGetActualBandwidth(newdev))) needBandwidthSet = true; @@ -3308,6 +3312,19 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; } + if (needHostLinkStateChange) { + if (newdev->hostlinkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (virNetDevSetOnline(newdev->ifname, false) < 0) + goto cleanup; + } else { + if (virNetDevSetOnline(newdev->ifname, true) < 0) + goto cleanup; + if (virNetDevIPInfoAddToDev(newdev->ifname, &newdev->hostIP) < 0) + goto cleanup; + } + needReplaceDevDef = true; + } + if (needReplaceDevDef) { /* the changes above warrant replacing olddev with newdev in * the domain's nets list. diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index d8a678b4ab13..f3afbdae4009 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -413,7 +413,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, { virMacAddr tapmac; int ret = -1; - unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; + unsigned int tap_create_flags = 0; bool template_ifname = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *tunpath = "/dev/net/tun"; @@ -427,6 +427,9 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, } } + if (net->hostlinkstate != VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) + tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP; + if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) || strchr(net->ifname, '%')) { @@ -453,9 +456,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, if (virNetDevSetMAC(net->ifname, &tapmac) < 0) goto cleanup; - if (virNetDevSetOnline(net->ifname, true) < 0) - goto cleanup; - if (net->script && virNetDevRunEthernetScript(net->ifname, net->script) < 0) goto cleanup; -- 2.9.3
On Tue, May 16, 2017 at 01:15:59 +0300, Vasiliy Tolstov wrote:
Signed-off-by: Vasiliy Tolstov <v.tolstov@selfip.ru> --- docs/formatdomain.html.in | 21 +++++++++++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_hotplug.c | 17 +++++++++++++++++ src/qemu/qemu_interface.c | 8 ++++---- 6 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8c884f4af9cb..dd8e6a4afa99 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5421,6 +5421,27 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.9.5</span> </p>
+ <h5><a name="elementHostLink">Modifying phisical link state</a></h5> +<pre> +... +<devices> + <interface type='ethernet'> + <source> + <b><link state='down'/></b> + <target dev='vnet0'/> + </interface> +</devices> +...</pre> + + <p> + This element provides means of setting state of the phisical network interface. + Possible values for attribute <code>state</code> are <code>up</code> and + <code>down</code>. If <code>down</code> is specified as the value, the interface + put in down state. Default behavior if this element is unspecified is to have the + link state <code>up</code>. + <span class="since">Since 3.3.2</span>
3.3.2? We dropped micro versions some time ago.
+ </p> + <h5><a name="mtu">MTU configuration</a></h5> <pre>
There already is an element called <link> with attribute "state" which modifies the link state in qemu. So that maps to guest link state. So this patch has a naming collision going on. I also don't really see how modifying the host side link state would be useful. Could you elaborate why you think it would be useful? At any rate, if you provide a justification, you also need to modify the XML portion to have a separate element and provide a test case as it's required with XML changes.
2017-05-16 9:32 GMT+03:00 Peter Krempa <pkrempa@redhat.com>:
3.3.2? We dropped micro versions some time ago.
This is copy/paste from link element, sorry.
+ </p> + <h5><a name="mtu">MTU configuration</a></h5> <pre>
There already is an element called <link> with attribute "state" which modifies the link state in qemu. So that maps to guest link state. So this patch has a naming collision going on.
Yes, but some times ago Laine Stump says that for host side link status we can use source element with link state.
I also don't really see how modifying the host side link state would be useful. Could you elaborate why you think it would be useful?
OSPF - tap device have addresses assigned to it and bird/quagga create routing based on interface link status and addresses on device. So if i have host side device in up state no matter that have guest - host system forward traffic to it. So i need to modify host side link status too.
At any rate, if you provide a justification, you also need to modify the XML portion to have a separate element and provide a test case as it's required with XML changes.
Can you gave me example test for xml changes? Thanks! -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On Tue, May 16, 2017 at 10:28:27AM +0300, Vasiliy Tolstov wrote:
2017-05-16 9:32 GMT+03:00 Peter Krempa <pkrempa@redhat.com>:
3.3.2? We dropped micro versions some time ago.
This is copy/paste from link element, sorry.
+ </p> + <h5><a name="mtu">MTU configuration</a></h5> <pre>
There already is an element called <link> with attribute "state" which modifies the link state in qemu. So that maps to guest link state. So this patch has a naming collision going on.
Yes, but some times ago Laine Stump says that for host side link status we can use source element with link state.
I also don't really see how modifying the host side link state would be useful. Could you elaborate why you think it would be useful?
OSPF - tap device have addresses assigned to it and bird/quagga create routing based on interface link status and addresses on device. So if i have host side device in up state no matter that have guest - host system forward traffic to it. So i need to modify host side link status too.
Shouldn't we just tie the host & guest link state together then. This doesn't seem like enough of a reason to introduce new XML elements. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
2017-05-16 11:16 GMT+03:00 Daniel P. Berrange <berrange@redhat.com>:
Shouldn't we just tie the host & guest link state together then. This doesn't seem like enough of a reason to introduce new XML elements.
Back to half year ago i'm already have such patch, but it reverted with message : link state only for guest side, for host side we need different element in xml. -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On 05/16/2017 04:21 AM, Vasiliy Tolstov wrote:
2017-05-16 11:16 GMT+03:00 Daniel P. Berrange <berrange@redhat.com>:
Shouldn't we just tie the host & guest link state together then. This doesn't seem like enough of a reason to introduce new XML elements.
Back to half year ago i'm already have such patch, but it reverted with message : link state only for guest side, for host side we need different element in xml.
I guess I should have been working through my backlog in reverse order. (I just responded to the message Vasiliy sent last week about this. At the time of the original discussion, I intended to send a patch to implement <source><link state='blah'/></source> as he's done here). Setting an interface online/offline can have further reaching sonsequences than just turning carrier on and off. It also removes and adds routes, IP addresses, and can trigger DHCP to request a new address, etc. Since we have other things separated for host vs guest side, it makes sense to do this for the link state as well. (tying two functions to a single knob often leads to problems, and once they're tied together, they can't be untied once you discover one of those problems. If there are two separate options, modifying both of them may be a bit more verbose, but it can still be done).
On 05/16/2017 06:40 PM, Laine Stump wrote:
On 05/16/2017 04:21 AM, Vasiliy Tolstov wrote:
2017-05-16 11:16 GMT+03:00 Daniel P. Berrange <berrange@redhat.com>:
Shouldn't we just tie the host & guest link state together then. This doesn't seem like enough of a reason to introduce new XML elements.
Back to half year ago i'm already have such patch, but it reverted with message : link state only for guest side, for host side we need different element in xml.
I guess I should have been working through my backlog in reverse order. (I just responded to the message Vasiliy sent last week about this. At the time of the original discussion, I intended to send a patch to implement <source><link state='blah'/></source> as he's done here).
Setting an interface online/offline can have further reaching sonsequences than just turning carrier on and off. It also removes and adds routes, IP addresses, and can trigger DHCP to request a new address, etc. Since we have other things separated for host vs guest side, it makes sense to do this for the link state as well. (tying two functions to a single knob often leads to problems, and once they're tied together, they can't be untied once you discover one of those problems. If there are two separate options, modifying both of them may be a bit more verbose, but it can still be done).
Oh, and I forgot to point out here (although I did in the original thread last fall) that a toplevel <link state='blah'/> historically only affects the state of the interface in the guest. If it began affecting the state of the tap device in the host as well, there would be unexpected (and undesireable) consequences. For example, if a tap device is attached to a bridge that has STP enabled, and you set the tap device link state down and then back up, the port will revert to "learning" mode, and won't begin forwarding packets again until the STP forward delay timer has expired. Likewise, if there is an IP address configured on the host side of the tap device and you set link state to down, any route associated with that device will also disappear, leading to route flapping if you have any sort of routing protocol running. Both of these would be undesirable side effects if, for example, all you wanted to do was trigger the guest to renegotiate an address with dhcp (by toggling the guest NIC's link state) (I'm sure there are other reasons someone might want to set the guest-side link state to down for a period without encountering the side effects I listed above, that's just what comes to mind first.) So even if we weren't concerned about the precendent of setting <link state='down'/> not affecting the tap device, it still makes sense to keep the two operations separate.
2017-05-17 4:25 GMT+03:00 Laine Stump <laine@laine.org>:
Oh, and I forgot to point out here (although I did in the original thread last fall) that a toplevel <link state='blah'/> historically only affects the state of the interface in the guest. If it began affecting the state of the tap device in the host as well, there would be unexpected (and undesireable) consequences.
So as i understand xml for this feature looks right. I found one minor issue, and send v2 shortly, but i want to know, does this will be accepted by upstream? -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On 05/17/2017 03:51 AM, Vasiliy Tolstov wrote:
2017-05-17 4:25 GMT+03:00 Laine Stump <laine@laine.org>:
Oh, and I forgot to point out here (although I did in the original thread last fall) that a toplevel <link state='blah'/> historically only affects the state of the interface in the guest. If it began affecting the state of the tap device in the host as well, there would be unexpected (and undesireable) consequences.
So as i understand xml for this feature looks right. I found one minor issue, and send v2 shortly, but i want to know, does this will be accepted by upstream?
Well, *I* think I've given sufficient reasons for having the two link states controlled separately, but since Dan and Peter had questioned its usefulness, we should see whether or not I've swayed their opinions :-) (BTW, sorry about dropping / completely forgetting about my patch for this back in August - this could have been resolved much sooner if I was better about keeping track of open branches.)
On Wed, May 17, 2017 at 08:16:58 -0400, Laine Stump wrote:
On 05/17/2017 03:51 AM, Vasiliy Tolstov wrote:
2017-05-17 4:25 GMT+03:00 Laine Stump <laine@laine.org>:
Oh, and I forgot to point out here (although I did in the original thread last fall) that a toplevel <link state='blah'/> historically only affects the state of the interface in the guest. If it began affecting the state of the tap device in the host as well, there would be unexpected (and undesireable) consequences.
So as i understand xml for this feature looks right. I found one minor issue, and send v2 shortly, but i want to know, does this will be accepted by upstream?
Well, *I* think I've given sufficient reasons for having the two link states controlled separately, but since Dan and Peter had questioned its usefulness, we should see whether or not I've swayed their opinions :-)
I think we should have two elements in this case. I just don't consider setting the host link side to be very useful.
2017-05-17 15:28 GMT+03:00 Peter Krempa <pkrempa@redhat.com>:
Well, *I* think I've given sufficient reasons for having the two link states controlled separately, but since Dan and Peter had questioned its usefulness, we should see whether or not I've swayed their opinions :-)
I think we should have two elements in this case. I just don't consider setting the host link side to be very useful.
Sorry, Laine. As i understand you already have alternative patch for such feature, can you share it? I'm really interesting how much it different from my. -- Vasiliy Tolstov, e-mail: v.tolstov@selfip.ru
On 05/18/2017 04:32 PM, Vasiliy Tolstov wrote:
2017-05-17 15:28 GMT+03:00 Peter Krempa <pkrempa@redhat.com>:
Well, *I* think I've given sufficient reasons for having the two link states controlled separately, but since Dan and Peter had questioned its usefulness, we should see whether or not I've swayed their opinions :-)
I think we should have two elements in this case. I just don't consider setting the host link side to be very useful.
Sorry, Laine. As i understand you already have alternative patch for such feature, can you share it? I'm really interesting how much it different from my.
After much searching, I finally found my patch, and learned that it was incomplete (I'd suspected that) so not of much use. The one thing I would say is that I had a prerequisite patch that changed the name of "linkstate" both in the virDomainNetDef and the temp local in the parsing function to "guestLinkState" so that it was more difficult to confuse the two. Aside from that, I looked through your patch and have a few comments the I will post. (it's not a thorough review, because I have a hard deadlines of tomorrow AM for a couple of completely unrelated (to libvirt, or even software development) tasks. (Protip - *never* volunteer to be the "parent advisor" for your kid's yearbook if you will be moving during the time that it all must be done. Also never store anything in heavy glass bottles directly over an electric stovetop that is itself made of glass).
On 05/15/2017 06:15 PM, Vasiliy Tolstov wrote:
Signed-off-by: Vasiliy Tolstov <v.tolstov@selfip.ru> --- docs/formatdomain.html.in | 21 +++++++++++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_hotplug.c | 17 +++++++++++++++++ src/qemu/qemu_interface.c | 8 ++++---- 6 files changed, 82 insertions(+), 4 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 8c884f4af9cb..dd8e6a4afa99 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -5421,6 +5421,27 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.9.5</span> </p>
+ <h5><a name="elementHostLink">Modifying phisical link state</a></h5> +<pre> +... +<devices> + <interface type='ethernet'> + <source> + <b><link state='down'/></b> + <target dev='vnet0'/> + </interface> +</devices> +...</pre> + + <p> + This element provides means of setting state of the phisical network interface.
It's not the physical device, it is the host side of the tap device that connects the guest to the network.
+ Possible values for attribute <code>state</code> are <code>up</code> and + <code>down</code>. If <code>down</code> is specified as the value, the interface + put in down state. Default behavior if this element is unspecified is to have the + link state <code>up</code>. + <span class="since">Since 3.3.2</span>
What pkrempa said. This will now be 3.4.0 (right? I can never keep track and I'm too lazy to look)
+ </p> + <h5><a name="mtu">MTU configuration</a></h5> <pre> ... diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 281309ec09da..89213d63b6e9 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2798,6 +2798,17 @@ All ip-related info for either the host or guest side of an interface --> <define name="interface-ip-info"> + <optional> + <element name="link"> + <attribute name="state"> + <choice> + <value>up</value> + <value>down</value> + </choice> + </attribute> + <empty/> + </element> + </optional> <zeroOrMore> <element name="ip"> <attribute name="address"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0ff216e3a373..b7398276af57 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -9606,6 +9606,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, char *devaddr = NULL; char *mode = NULL; char *linkstate = NULL;
As I said in my other mail, I had renamed "linkstate" to "guestLinkState" in a separate patch to make it clearer which was for what. If you do that, do it in a separate patch.
+ char *hostlinkstate = NULL;
I prefer hostLinkState, but the naming in this function is all mixed up anyway...
char *addrtype = NULL; char *domain_name = NULL; char *vhostuser_mode = NULL; @@ -9654,6 +9655,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, if (virDomainNetIPInfoParseXML(_("interface host IP"), ctxt, &def->hostIP) < 0) goto error; + + if (!hostlinkstate) + hostlinkstate = virXPathString("string(./link/@state)", ctxt);
The parsing for guest link state uses virXMLPropString() (as does the parsing for just about everything else inside <source>), but I like this better anyway (virXPathBlah() may be less efficient, but I think we should use it more).
+ ctxt->node = tmpnode; } if (!macaddr && xmlStrEqual(cur->name, BAD_CAST "mac")) { @@ -10303,6 +10308,16 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, } }
+ def->hostlinkstate = VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DEFAULT;
DEFAULT is 0, so there isn't any need to explicitly set it.
+ if (hostlinkstate != NULL) { + if ((def->hostlinkstate = virDomainNetInterfaceLinkStateTypeFromString(hostlinkstate)) <= 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unknown interface link state '%s'"), + hostlinkstate); + goto error; + } + } +
You need to validate that it isn't set for interface type='hostdev' (and maybe some other types?
if (filter != NULL) { switch (def->type) { case VIR_DOMAIN_NET_TYPE_ETHERNET: @@ -10371,6 +10386,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, VIR_FREE(devaddr); VIR_FREE(mode); VIR_FREE(linkstate); + VIR_FREE(hostlinkstate); VIR_FREE(addrtype); VIR_FREE(domain_name); VIR_FREE(trustGuestRxFilters); @@ -22113,6 +22129,18 @@ virDomainNetDefFormat(virBufferPtr buf, break; }
+ if (def->hostlinkstate) { + if (sourceLines == 0) { + virBufferAddLit(buf, "<source>\n"); + sourceLines += 2; + } + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<link state='%s'/>\n", + virDomainNetInterfaceLinkStateTypeToString(def->hostlinkstate)); + virBufferAdjustIndent(buf, -2); + sourceLines += 2; + } +
Maybe put this *below* this following comment instead of above it
/* if sourceLines == 0 - no <source> info at all so far * sourceLines == 1 - first line written, no terminating ">" * sourceLines > 1 - multiple lines, including subelements diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 09fb7aada4b2..71e12a30c2c1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1037,6 +1037,7 @@ struct _virDomainNetDef { virNetDevVlan vlan; int trustGuestRxFilters; /* enum virTristateBool */ int linkstate; + int hostlinkstate;
Again, I like hostLinkState (I looked in HACKING and don't see any opinion of all lowercase vs camelCase vs underscores for struct members or locals though).
unsigned int mtu; virNetDevCoalescePtr coalesce; }; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index e8d29186eb32..7fc41b28d9f8 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2978,6 +2978,7 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, bool needBridgeChange = false; bool needFilterChange = false; bool needLinkStateChange = false; + bool needHostLinkStateChange = false; bool needReplaceDevDef = false; bool needBandwidthSet = false; int ret = -1; @@ -3264,6 +3265,9 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, if (olddev->linkstate != newdev->linkstate) needLinkStateChange = true;
+ if (olddev->hostlinkstate != newdev->hostlinkstate) + needHostLinkStateChange = true; + if (!virNetDevBandwidthEqual(virDomainNetGetActualBandwidth(olddev), virDomainNetGetActualBandwidth(newdev))) needBandwidthSet = true; @@ -3308,6 +3312,19 @@ qemuDomainChangeNet(virQEMUDriverPtr driver, goto cleanup; }
+ if (needHostLinkStateChange) { + if (newdev->hostlinkstate == VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) { + if (virNetDevSetOnline(newdev->ifname, false) < 0) + goto cleanup; + } else { + if (virNetDevSetOnline(newdev->ifname, true) < 0) + goto cleanup; + if (virNetDevIPInfoAddToDev(newdev->ifname, &newdev->hostIP) < 0) + goto cleanup;
(I probably would have forgotten to do this :-P)
+ } + needReplaceDevDef = true; + } + if (needReplaceDevDef) { /* the changes above warrant replacing olddev with newdev in * the domain's nets list. diff --git a/src/qemu/qemu_interface.c b/src/qemu/qemu_interface.c index d8a678b4ab13..f3afbdae4009 100644 --- a/src/qemu/qemu_interface.c +++ b/src/qemu/qemu_interface.c @@ -413,7 +413,7 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, { virMacAddr tapmac; int ret = -1; - unsigned int tap_create_flags = VIR_NETDEV_TAP_CREATE_IFUP; + unsigned int tap_create_flags = 0; bool template_ifname = false; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); const char *tunpath = "/dev/net/tun"; @@ -427,6 +427,9 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, } }
+ if (net->hostlinkstate != VIR_DOMAIN_NET_INTERFACE_LINK_STATE_DOWN) + tap_create_flags |= VIR_NETDEV_TAP_CREATE_IFUP; + if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_TAP_PREFIX) || strchr(net->ifname, '%')) { @@ -453,9 +456,6 @@ qemuInterfaceEthernetConnect(virDomainDefPtr def, if (virNetDevSetMAC(net->ifname, &tapmac) < 0) goto cleanup;
- if (virNetDevSetOnline(net->ifname, true) < 0) - goto cleanup; -
You need to look at the commit log message for the commit that added the previous two lines - 07262221 - virNetdevTapCreate() doesn't honor the VIR_NETDEV_TAP_CREATE_IFUP flag (only virNetdevTapCreateInBridgePort() does). I don't recall why I decided to fix the bug by just setting the device online explicitly in qemuInterfaceEthernetConnect() rather than changing virNetdevTapCreate(); my guess is that modifying virNetdevTapCreate() directly would have caused side effects elsewhere (but when I look now, I see the only other place it's called from is virNetdevTapCreateInBridgePort(), which could easily be modified to send "flags & ~VIR_NETDEV_TAP_CREATE_IFUP" to virNetdevTapCreate(), then set the device online itself. (All of that fixup should be in a separate prerequisite patch though, not in this one)
if (net->script && virNetDevRunEthernetScript(net->ifname, net->script) < 0) goto cleanup;
You haven't done anything to honor the host link state for tap devices that are connected to bridges, or for macvtap devices. Both of those need similar support. BTW, Cc me on v2 so I'll notice it sooner :-)
participants (4)
-
Daniel P. Berrange -
Laine Stump -
Peter Krempa -
Vasiliy Tolstov