Re: [libvirt] [PATCH v2] qemu: Adding 'downscript' feature for QEMU network interfaces.

On 05/05/2017 10:22 AM, Julio Faracco wrote: > This commit adds the support for 'downscript' feature: > - For QEMU command line with the option: > '-net downscript=/etc/qemu-ifdown,...'. > > - For Domains with a network interface description: > '<interface type='ethernet'> > ... > <downscript path='/etc/qemu-ifdown'/> > ... > </interface>' > > The options 'script' and 'downscript' accept the argument 'no' to disable > the script executions. The way that the code was implemented, the XML file > accepts '<[down]script path='no'>' to solve this problem. > > This commit updates the tests and documentation too. > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=825939 I have a couple issues with this implementation: 1) the script in an interface's <script> element is executed by libvirt itself, not by qemu (historically - it used to be executed by qemu, but this was changed a few years ago as part of work to make <interface type='ethernet'> usable without running qemu as root). Execution by libvirt vs by qemu makes a *big* difference, since the qemu process is generally running unprivileged with all capabilities dropped (i.e. the script probably wouldn't have sufficient permission to do what a lot of people would want to do). 2) the <downscript> (and also the existing <script>) only get the name of the tap device as a parameter on their commandline, but it's likely that people would often want more information about the domain and its configuration in order to do some intelligent things in the script. Rather than propagating a limited model, maybe we should think about deprecating <script path='blah'/> (it would still work, but would get a better replacement) with something like

<script up='Bob' down='Loblaw'/>

(this is just a point for discussion - don't go off an implement it that way just because I suggested it! Somebody else may disagree...). What I'm thinking is that these scripts could take arguments similar to the network hooks, including the full domain XML and interface XML on stdin. (the problem with network hooks and <interface type='ethernet'> is of course that network hooks aren't executed for type='ethernet'> (maybe that should change, although I like the idea of specifying the name of the script in the XML, rather than having a network hook that is tried for *every single interface in every domain* (and thus taints all domains on the host)).

> > Signed-off-by: Julio Faracco <jcfaracco(a)gmail.com&gt; > --- > docs/formatdomain.html.in | 1 + > docs/schemas/domaincommon.rng | 8 ++++++++ > src/conf/domain_conf.c | 13 +++++++++++++ > src/conf/domain_conf.h | 1 + > src/qemu/qemu_parse_command.c | 4 ++++ > tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.args | 2 +- > tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.xml | 1 + > tests/qemuargv2xmldata/qemuargv2xml-net-eth.args | 2 +- > tests/qemuargv2xmldata/qemuargv2xml-net-eth.xml | 1 + > tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml | 1 + > tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml | 1 + > tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-ifname.xml | 1 + > tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth.xml | 1 + > 13 files changed, 35 insertions(+), 2 deletions(-) > > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 8c884f4..89fe86d 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -4663,6 +4663,7 @@ > &lt;interface type='ethernet'&gt; > &lt;target dev='vnet7'/&gt; > &lt;script path='/etc/qemu-ifup-mynet'/&gt; > + &lt;downscript path='/etc/qemu-ifdown-mynet'/&gt; > &lt;/interface&gt; > &lt;/devices&gt; > ...</pre> > diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng > index 281309e..2f88dda 100644 > --- a/docs/schemas/domaincommon.rng > +++ b/docs/schemas/domaincommon.rng > @@ -2609,6 +2609,14 @@ > </element> > </optional> > <optional> > + <element name="downscript"> > + <attribute name="path"> > + <ref name="filePath"/> > + </attribute> > + <empty/> > + </element> > + </optional> > + <optional> > <element name="backenddomain"> > <attribute name="name"> > <ref name="domainName"/> > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 0ff216e..32d5720 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -1935,6 +1935,7 @@ virDomainNetDefClear(virDomainNetDefPtr def) > VIR_FREE(def->backend.vhost); > VIR_FREE(def->virtPortProfile); > VIR_FREE(def->script); > + VIR_FREE(def->downscript); > VIR_FREE(def->domain_name); > VIR_FREE(def->ifname); > VIR_FREE(def->ifname_guest); > @@ -9589,6 +9590,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, > char *ifname_guest = NULL; > char *ifname_guest_actual = NULL; > char *script = NULL; > + char *downscript = NULL; > char *address = NULL; > char *port = NULL; > char *localaddr = NULL; > @@ -9761,6 +9763,9 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, > } else if (!script && > xmlStrEqual(cur->name, BAD_CAST "script")) { > script = virXMLPropString(cur, "path"); > + } else if (!downscript && > + xmlStrEqual(cur->name, BAD_CAST "downscript")) { > + downscript = virXMLPropString(cur, "path"); > } else if (!domain_name && > xmlStrEqual(cur->name, BAD_CAST "backenddomain")) { > domain_name = virXMLPropString(cur, "name"); > @@ -10074,6 +10079,10 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, > def->script = script; > script = NULL; > } > + if (downscript != NULL) { > + def->downscript = downscript; > + downscript = NULL; > + } > if (domain_name != NULL) { > def->domain_name = domain_name; > domain_name = NULL; > @@ -10356,6 +10365,7 @@ virDomainNetDefParseXML(virDomainXMLOptionPtr xmlopt, > VIR_FREE(dev); > virDomainActualNetDefFree(actual); > VIR_FREE(script); > + VIR_FREE(downscript); > VIR_FREE(bridge); > VIR_FREE(model); > VIR_FREE(backend); > @@ -22158,6 +22168,9 @@ virDomainNetDefFormat(virBufferPtr buf, > > virBufferEscapeString(buf, "<script path='%s'/>\n", > def->script); > + if (def->downscript) > + virBufferEscapeString(buf, "<downscript path='%s'/>\n", > + def->downscript); > virBufferEscapeString(buf, "<backenddomain name='%s'/>\n", def->domain_name); > > if (def->ifname && > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 09fb7aa..9deca76 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -1024,6 +1024,7 @@ struct _virDomainNetDef { > unsigned long sndbuf; > } tune; > char *script; > + char *downscript; > char *domain_name; /* backend domain name */ > char *ifname; /* interface name on the host (<target dev='x'/>) */ > virNetDevIPInfo hostIP; > diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c > index af9063c..d773917 100644 > --- a/src/qemu/qemu_parse_command.c > +++ b/src/qemu/qemu_parse_command.c > @@ -1060,6 +1060,10 @@ qemuParseCommandLineNet(virDomainXMLOptionPtr xmlopt, > def->script = values[i]; > values[i] = NULL; > } else if (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET && > + STREQ(keywords[i], "downscript") && STRNEQ(values[i], "")) { > + def->downscript = values[i]; > + values[i] = NULL; > + } else if (def->type == VIR_DOMAIN_NET_TYPE_ETHERNET && > STREQ(keywords[i], "ifname")) { > def->ifname = values[i]; > values[i] = NULL; > diff --git a/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.args b/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.args > index 4d74ae4..5d7129c 100644 > --- a/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.args > +++ b/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.args > @@ -18,6 +18,6 @@ QEMU_AUDIO_DRV=none \ > -usb \ > -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=ide,bus=0,unit=0 \ > -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=rtl8139,name=net0 \ > --net tap,ifname=nic02,script=/etc/qemu-ifup,vlan=0,name=hostnet0 \ > +-net tap,ifname=nic02,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown,vlan=0,name=hostnet0 \ > -serial none \ > -parallel none > diff --git a/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.xml b/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.xml > index fa9a892..8e04efb 100644 > --- a/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.xml > +++ b/tests/qemuargv2xmldata/qemuargv2xml-net-eth-ifname.xml > @@ -30,6 +30,7 @@ > <interface type='ethernet'> > <mac address='00:11:22:33:44:55'/> > <script path='/etc/qemu-ifup'/> > + <downscript path='/etc/qemu-ifdown'/> > <target dev='nic02'/> > <model type='rtl8139'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > diff --git a/tests/qemuargv2xmldata/qemuargv2xml-net-eth.args b/tests/qemuargv2xmldata/qemuargv2xml-net-eth.args > index 89eb4c1..0e3fa21 100644 > --- a/tests/qemuargv2xmldata/qemuargv2xml-net-eth.args > +++ b/tests/qemuargv2xmldata/qemuargv2xml-net-eth.args > @@ -18,6 +18,6 @@ QEMU_AUDIO_DRV=none \ > -usb \ > -drive file=/dev/HostVG/QEMUGuest1,format=raw,if=ide,bus=0,unit=0 \ > -net nic,macaddr=00:11:22:33:44:55,vlan=0,model=rtl8139,name=net0 \ > --net tap,script=/etc/qemu-ifup,vlan=0,name=hostnet0 \ > +-net tap,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown,vlan=0,name=hostnet0 \ > -serial none \ > -parallel none > diff --git a/tests/qemuargv2xmldata/qemuargv2xml-net-eth.xml b/tests/qemuargv2xmldata/qemuargv2xml-net-eth.xml > index 57c4be8..d177ca8 100644 > --- a/tests/qemuargv2xmldata/qemuargv2xml-net-eth.xml > +++ b/tests/qemuargv2xmldata/qemuargv2xml-net-eth.xml > @@ -30,6 +30,7 @@ > <interface type='ethernet'> > <mac address='00:11:22:33:44:55'/> > <script path='/etc/qemu-ifup'/> > + <downscript path='/etc/qemu-ifdown'/> > <model type='rtl8139'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > </interface> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml > index dd0d752..21d8259 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth-ifname.xml > @@ -26,6 +26,7 @@ > <interface type='ethernet'> > <mac address='00:11:22:33:44:55'/> > <script path='/etc/qemu-ifup'/> > + <downscript path='/etc/qemu-ifdown'/> > <target dev='nic02'/> > <model type='rtl8139'/> > </interface> > diff --git a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml > index 48acadf..9f40122 100644 > --- a/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml > +++ b/tests/qemuxml2argvdata/qemuxml2argv-net-eth.xml > @@ -26,6 +26,7 @@ > <interface type='ethernet'> > <mac address='00:11:22:33:44:55'/> > <script path='/etc/qemu-ifup'/> > + <downscript path='/etc/qemu-ifdown'/> > <model type='rtl8139'/> > </interface> > <input type='mouse' bus='ps2'/> > diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-ifname.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-ifname.xml > index c36baa0..b71fd5a 100644 > --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-ifname.xml > +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth-ifname.xml > @@ -30,6 +30,7 @@ > <interface type='ethernet'> > <mac address='00:11:22:33:44:55'/> > <script path='/etc/qemu-ifup'/> > + <downscript path='/etc/qemu-ifdown'/> > <target dev='nic02'/> > <model type='rtl8139'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth.xml > index 898bda6..c55cd68 100644 > --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth.xml > +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-net-eth.xml > @@ -30,6 +30,7 @@ > <interface type='ethernet'> > <mac address='00:11:22:33:44:55'/> > <script path='/etc/qemu-ifup'/> > + <downscript path='/etc/qemu-ifdown'/> > <model type='rtl8139'/> > <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> > </interface> >