On 05/04/2012 12:51 PM, Laine Stump wrote: > Until now, the nl_pid of the source address of every message sent by > virNetlinkCommand has been set to the value of getpid(). Most of the > time this doesn't matter, and in the one case where it does > (communication with lldpad), it previously was the proper thing to do, > because the netlink event service (which listens on a netlink socket > for unsolicited messages from lldpad) coincidentally always happened > to bind with a local nl_pid == getpid(). > > With the fix for: > > https://bugzilla.redhat.com/show_bug.cgi?id=816465 > > that particular nl_pid is now effectively a reserved value, so the > netlink event service will always bind to something else > (coincidentally "getpid() + (1 << 22)", but it really could be > anything). The result is that communication between lldpad and > libvirtd is broken (lldpad gets a "disconnected" error when it tries > to send a directed message). Thanks for the detailed commit message - it will be a lifesaver looking back at this patch down the road. > > The solution to this problem cause by a solution, is to query the s/cause/caused/ > netlink event service's nlhandle for its "local_port", and send that > as the source nl_pid (but only when sending to lldpad, of course - in > other cases we maintain the old behavior of sending getpid()). > > There are two cases where a message is being directed at lldpad - one > in virNetDevLinkDump, and one in virNetDevVPortProfileOpSetLink. > > The case of virNetDevVPortProfileOpSetLink is simplest to explain - > only if !nltarget_kernel, i.e. the message isn't targetted for the > kernel, is the dst_pid set (by calling > virNetDevVPortProfileGetLldpadPid()), so only in that case do we call > virNetlinkEvenServiceLocalPid() to set src_pid. s/EvenService/EventService/ > > For virNetDevLinkDump, it's a bit more complicated. The call to > virNetDevVPortProfileGetLldpadPid() was effectively up one level (in > virNetDevVPortProfileOpCommon), although obscured by an unnecessary > passing of a function pointer. This patch removes the function > pointer, and calls virNetDevVPortProfileGetLldpadPid() directly in > virNetDevVPortProfileOpCommon - if it's doing this, it knows that it > should also call virNetlinkEventServiceLocalPid() to set src_pid too; > then it just passes src_pid and dst_pid down to > virNetDevLinkDump. Since (src_pid == 0 && dst_pid == 0) implies that > the kernel is the destination, there is no longer any need to send > nltarget_kernel as an arg to virNetDevLinkDump, so it's been removed. > > The disparity between src_pid being int and dst_pid being uint32_t may > be a bit disconcerting to some, but I didn't want to complicate > virNetlinkEventServiceLocalPid() by having status returned separately > from the value. I can live with the difference; after all, pid_t is a signed type, precisely because negative pids have special meaning in POSIX, so we already have issues if the most significant bit of a variable labeled 'uint32_t pid' is set. I suppose you could have also used pid==0 as error instead of worrying about int, but what you have is reasonable. ACK to the code aspects; but wait for the test results before pushing. -- Eric Blake eblake@xxxxxxxxxx +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list