On 05/15/2017 06:15 PM, Vasiliy Tolstov wrote:
> Signed-off-by: Vasiliy Tolstov <v.tolstov@xxxxxxxxx>
> ---
> 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 :-)
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
