[libvirt] [PATCHv3 4/4] util: set src_pid for virNetlinkCommand when appropriate

[Eric Blake]

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 at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120504/0729606a/attachment-0001.sig>