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

[Laine Stump]

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.