Re: [PATCH] rpcbind: add support for systemd socket activation

[Chuck Lever <chuck.lever@oracle.com>]

On Feb 1, 2012, at 4:45 PM, Lennart Poettering wrote:

> On Wed, 01.02.12 17:37, Tom Gundersen (teg@jklm.no) wrote:
> 
>>>> +     /* Try to find if one of the systemd sockets we were given match
>>>> +      * our netconfig structure. */
>>>> +
>>>> +     for (fd = SD_LISTEN_FDS_START; fd < SD_LISTEN_FDS_START + n; fd++) {
>>>> +             struct __rpc_sockinfo si_other;
>>>> +             union {
>>>> +                     struct sockaddr sa;
>>>> +                     struct sockaddr_un un;
>>>> +                     struct sockaddr_in in4;
>>>> +                     struct sockaddr_in6 in6;
>>>> +                     struct sockaddr_storage storage;
>>>> +             } sa;
>>> 
>>> Why is sockaddr_storage included in this union?
>> 
>> This is from Lennart's original patch. Lennart, care to comment?
> 
> Well, simply because sockaddr_storage is the actual struct one should
> normally use for this kind of thing (where you try to determine a
> sockaddr from a socket you don't know at all). With one exception
> however, sockaddr_un is actually longer than sockaddr_storage, which is
> documented borkedness in the socket API.

You don't reference any of the fields inside this union, except for sa.  It seems unnecessary to include all of these members, and then not use most of them.

The biggest address you're ever going to get out of libtirpc is a sockaddr_storage.  If we must stick with a union to prevent pointer aliasing, can we have just two members: sockaddr and sockaddr_storage?

Otherwise, there's no need for this kind of generality here:  TI-RPC handles only IPv4, IPv6, and AF_LOCAL.

-- 
Chuck Lever
chuck[dot]lever[at]oracle[dot]com