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

On 02/13/2014 03:17 PM, Michal Privoznik wrote: > 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. (Actually, I hadn't said that I thought it was a bad idea to check the return value, only that I hadn't thought of it :-) > >> >> >> 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 :) Okay, since my main objection is the lack of XML for the specific interface in the "plugged" event, and since the interface XML anyway wouldn't be useful until I fix it to contain the actual allocated device info (as described in the remainder of my earlier message), I say ACK to this patch (with the couple of grammar fixes I'd mentioned earlier), so the entire series is now ACKed. After you've pushed these patches, I will fix the live <interface> XML (I'm working on that already, and when it is fixed I'll add it to the XML that is sent for the plugged and unplugged hooks.