Re: [libvirt] [PATCH v4 2/3] network: Introduce network hooks

On 13.02.2014 12:10, Laine Stump wrote: > > On 02/12/2014 08:20 PM, Laine Stump wrote: >> It all looks fine (aside from the few small grammar corrections in >> the docs. I just want one last confirmation that we don't want both >> "plug" and "plugged" events, and in that case ACK. > > A few marginally related things came to my mind since I sent that reply, > leading me to rethink my conditional ACK: > > 1) ==================================================== > > I looked back through the review comments of previous versions, and see > that I mis-remembered about anyone objecting to both a "plug" and a > "plugged" hook. I wasn't really thinking in terms of the "plug" hook > being able to refuse plugging in an interface, but more that there my be > some operations that must be performed prior to allocating the > tap/macvtap device, and also that it makes it more consistent with the > other hook groups (domains and networks both have "start" "started" > "stopped", so to me it seems consistent for a network interface to have > "plug" "plugged" "unplugged"). Daniel - care to reel in my > over-ambitious spirit here? :-) Okay, we can ignore the hook return value in (un-)plug case. I don't really care.

> > > 2) ==================================================== > > Beyond that, I was thinking about this last night as I fell asleep and > realized that in these plug hooks, there is no simple way to determine > which interface of the domain is being plugged/unplugged - we are > sending the full domain XML and full network XML, but if the domain has > multiple interfaces we can't easily figure out which is the one we're > plugging (and as a matter of fact, due to the way > qemuDomainAttachNetDevice() is organized, the new device being plugged > in will not yet be in the domain XML *at all* at the time the "plugged" > hook is called![*]) > > So I think we need to add the specific <interface> to the XML sent to > the hook, i.e.: > > <hookData> > <interface> > ... > </interface> > <network> > ... > </network> > <domain> > ... > </domain> > <hookData> > > (Putting the lone <interface> first will make it simpler for luddites > like me to just grep for the first occurence of "<mac address" on stdin > rather than firing up a full-fledged XML parser.) Well, the hook script is now fed with XML - we can do whatever we want - even after this is pushed :)