On 18.02.2014 12:20, Laine Stump wrote:
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.
Okay, I've pushed this. Thanks.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list