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

On Tue, May 16, 2017 at 06:16:19PM -0400, Laine Stump wrote: > 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. I agree that we could/should pass more info, but I also don't think we need to change the existing syntax. In particular I think we should allow one script to be be used to handle both up & down events. We just need to pass an event name to the script - of course we need a way to opt-in to this otherwise we break existing scripts. So how about <script path="/some/path" event="on|off"/> If 'event=off', then we only ever run it on up. If 'event=on', then we allow running it multiple times, with the 2nd argument being the event name. Initially we can have "up" and "down" as valid event names, but document that apps must expect further events later.

As for passing Domain name/uuid/id, we can do that unconditionally by just setting env vars regardless of anything else.