On 05/17/2017 11:26 AM, Daniel P. Berrange wrote: > 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. Yeah, I like that better than my idea or the original - it makes future expansion much simpler. (Note that it's not really well defined when the up script is called, nor when the down script is called. When qemu used to execute the up script, it was called "at some unknown time after qemu created the tap device", and now it is called 1) after the tap device is created, MAC address set, and set online, but 2) before nwfilter or macFilter rules are applied. A down script might need to be called before qemu exits (because that automatically destroys the tap device, which is a problem if the tap device needs to exist in order for the script to run properly), or it might need to be called *after* qemu exits (e.g. it might be removing some iptables rule that opens an unwanted hole if the device still exists). Truthfully I'm not sure exactly when is the most useful time.) > As for passing Domain name/uuid/id, we can do that unconditionally > by just setting env vars regardless of anything else. This sounds good too. Also much simpler for a script to deal with than parsing the full XML, although it has the danger that a script may want something obscure from the XML that we haven't had the forethought to put in an environment variable (and since you can't call the libvirt API from within a script, they wouldn't even be able to read the XML via a virsh dumpxml etc). What about the hybrid of doing both? (env vars and domain+interface XML in stdin to the script) It could be overkill, but if someone ends up needing it, adding now (and tying it to the same "event='on|off'") would be less disruptive than trying to add it later. (Or maybe items we find a need for in the future could just be added to the environment). -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
