Re: [PATCH 02/16] lockd: address-family independent printable addresses

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

On Tue, Jul 15, 2008 at 4:25 PM, J. Bruce Fields <bfields@fieldses.org> wrote:
> On Mon, Jun 30, 2008 at 06:58:22PM -0400, Chuck Lever wrote:
>> Knowing which source address is used for communicating with remote NLM
>> services can be helpful for debugging configuration problems on hosts
>> with multiple addresses.
>>
>> Keep the dprintk debugging here, but adapt it so it displays AF_INET6
>> addresses properly.  There are also a couple of dprintk clean-ups as
>> well.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>>
>>  fs/lockd/host.c             |   79 +++++++++++++++++++++++++++++++------------
>>  include/linux/lockd/lockd.h |    3 ++
>>  2 files changed, 60 insertions(+), 22 deletions(-)
>>
>>
>> diff --git a/fs/lockd/host.c b/fs/lockd/host.c
>> index a17664c..350d757 100644
>> --- a/fs/lockd/host.c
>> +++ b/fs/lockd/host.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/types.h>
>>  #include <linux/slab.h>
>>  #include <linux/in.h>
>> +#include <linux/in6.h>
>>  #include <linux/sunrpc/clnt.h>
>>  #include <linux/sunrpc/svc.h>
>>  #include <linux/lockd/lockd.h>
>> @@ -38,6 +39,28 @@ static struct nsm_handle * nsm_find(const struct sockaddr_in *sin,
>>                                        const char *hostname,
>>                                        unsigned int hostname_len);
>>
>> +static void nlm_display_address(const struct sockaddr *sap,
>> +                             char *buf, const size_t len)
>> +{
>> +     const struct sockaddr_in *sin = (struct sockaddr_in *)sap;
>> +     const struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *)sap;
>> +
>> +     switch (sap->sa_family) {
>> +     case AF_UNSPEC:
>> +             snprintf(buf, len, "unspecified");
>> +             break;
>> +     case AF_INET:
>> +             snprintf(buf, len, NIPQUAD_FMT, NIPQUAD(sin->sin_addr.s_addr));
>> +             break;
>> +     case AF_INET6:
>> +             snprintf(buf, len, NIP6_FMT, NIP6(sin6->sin6_addr));
>> +             break;
>> +     default:
>> +             snprintf(buf, len, "unsupported address family");
>> +             break;
>> +     }
>> +}
>
> Is there code we could reuse from someplace else?

In this case, we need to differentiate between "unspecified" and
"unsupported" addresses (see nlmclnt_ vs. nlmsvc_lookup).  The other
places don't need this distinction.  Each one of these has slightly
different details and requirements.

We have the same issue with setting and retrieving port numbers.

I think once the IPv6 support is integrated and the dust has settled
we will find plenty of places where refactoring would result in
smaller and more shared code.  The problem is picking a common place
to house all these little bits and pieces.

>
>> +
>>  /*
>>   * Common host lookup routine for server & client
>>   */
>> @@ -54,14 +77,10 @@ static struct nlm_host *nlm_lookup_host(int server,
>>       struct nsm_handle *nsm = NULL;
>>       int             hash;
>>
>> -     dprintk("lockd: nlm_lookup_host("NIPQUAD_FMT"->"NIPQUAD_FMT
>> -                     ", p=%d, v=%u, my role=%s, name=%.*s)\n",
>> -                     NIPQUAD(ssin->sin_addr.s_addr),
>> -                     NIPQUAD(sin->sin_addr.s_addr), proto, version,
>> -                     server? "server" : "client",
>> -                     hostname_len,
>> -                     hostname? hostname : "<none>");
>> -
>> +     dprintk("lockd: nlm_lookup_host(proto=%d, vers=%u,"
>> +                     " my role is %s, hostname=%.*s)\n",
>> +                     proto, version, server? "server" : "client",
>> +                     hostname_len, hostname? hostname : "<none>");
>>
>>       hash = NLM_ADDRHASH(sin->sin_addr.s_addr);
>>
>> @@ -101,22 +120,31 @@ static struct nlm_host *nlm_lookup_host(int server,
>>               hlist_add_head(&host->h_hash, chain);
>>
>>               nlm_get_host(host);
>> +             dprintk("lockd: nlm_lookup_host found host %s (%s)\n",
>> +                             host->h_name, host->h_addrbuf);
>>               goto out;
>>       }
>> -     if (nsm)
>> -             atomic_inc(&nsm->sm_count);
>>
>> -     host = NULL;
>> -
>> -     /* Sadly, the host isn't in our hash table yet. See if
>> -      * we have an NSM handle for it. If not, create one.
>> +     /*
>> +      * The host wasn't in our hash table.  If we don't
>> +      * have an NSM handle for it yet, create one.
>>        */
>> -     if (!nsm && !(nsm = nsm_find(sin, hostname, hostname_len)))
>> -             goto out;
>> +     if (nsm)
>> +             atomic_inc(&nsm->sm_count);
>> +     else {
>> +             host = NULL;
>> +             nsm = nsm_find(sin, hostname, hostname_len);
>> +             if (!nsm) {
>> +                     dprintk("lockd: nlm_lookup_host failed; "
>> +                             "no nsm handle\n");
>> +                     goto out;
>> +             }
>> +     }
>
> This seems like an odd mixture of unrelated changes; I've lost track of
> the rationale.  Maybe split out another patch?

I noticed this the other day, and I can't remember why I had to move
this hunk into this patch.  Normally I would immediately agree to this
request, but I am supposed to be on vacation this week :-)

More below...

>>
>>       host = kzalloc(sizeof(*host), GFP_KERNEL);
>>       if (!host) {
>>               nsm_release(nsm);
>> +             dprintk("lockd: nlm_lookup_host failed; no memory\n");
>>               goto out;
>>       }
>>       host->h_name       = nsm->sm_name;
>> @@ -143,6 +171,15 @@ static struct nlm_host *nlm_lookup_host(int server,
>>       INIT_LIST_HEAD(&host->h_reclaim);
>>
>>       nrhosts++;
>> +
>> +     nlm_display_address((struct sockaddr *)&host->h_addr,
>> +                             host->h_addrbuf, sizeof(host->h_addrbuf));
>> +     nlm_display_address((struct sockaddr *)&host->h_saddr,
>> +                             host->h_saddrbuf, sizeof(host->h_saddrbuf));
>> +
>> +     dprintk("lockd: nlm_lookup_host created host %s\n",
>> +                     host->h_name);
>> +
>>  out:
>>       mutex_unlock(&nlm_host_mutex);
>>       return host;
>> @@ -207,9 +244,8 @@ nlm_bind_host(struct nlm_host *host)
>>  {
>>       struct rpc_clnt *clnt;
>>
>> -     dprintk("lockd: nlm_bind_host("NIPQUAD_FMT"->"NIPQUAD_FMT")\n",
>> -                     NIPQUAD(host->h_saddr.sin_addr),
>> -                     NIPQUAD(host->h_addr.sin_addr));
>> +     dprintk("lockd: nlm_bind_host %s (%s), my addr=%s\n",
>> +                     host->h_name, host->h_addrbuf, host->h_saddrbuf);
>>
>>       /* Lock host handle */
>>       mutex_lock(&host->h_mutex);
>> @@ -221,7 +257,7 @@ nlm_bind_host(struct nlm_host *host)
>>               if (time_after_eq(jiffies, host->h_nextrebind)) {
>>                       rpc_force_rebind(clnt);
>>                       host->h_nextrebind = jiffies + NLM_HOST_REBIND;
>> -                     dprintk("lockd: next rebind in %ld jiffies\n",
>> +                     dprintk("lockd: next rebind in %lu jiffies\n",
>>                                       host->h_nextrebind - jiffies);
>>               }
>>       } else {
>> @@ -324,8 +360,7 @@ void nlm_host_rebooted(const struct sockaddr_in *sin,
>>       struct nsm_handle *nsm;
>>       struct nlm_host *host;
>>
>> -     dprintk("lockd: nlm_host_rebooted(%s, %u.%u.%u.%u)\n",
>> -                     hostname, NIPQUAD(sin->sin_addr));
>> +     dprintk("lockd: nlm_host_rebooted(%s)\n", hostname);

I've re-written this piece to keep the display of the address.  I
think that would continue to be a valuable piece of debugging
information.

If you're about to include this patch, I'd like to send you the new
version of this patch and the later ones that change as a result.

>>
>>       /* Find the NSM handle for this peer */
>>       if (!(nsm = __nsm_find(sin, hostname, hostname_len, 0)))
>> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
>> index a6d1215..5774d6e 100644
>> --- a/include/linux/lockd/lockd.h
>> +++ b/include/linux/lockd/lockd.h
>> @@ -61,6 +61,9 @@ struct nlm_host {
>>       struct list_head        h_granted;      /* Locks in GRANTED state */
>>       struct list_head        h_reclaim;      /* Locks in RECLAIM state */
>>       struct nsm_handle *     h_nsmhandle;    /* NSM status handle */
>> +
>> +     char                    h_addrbuf[48],  /* address eyecatchers */
>> +                             h_saddrbuf[48];
>>  };
>>
>>  struct nsm_handle {

-- 
Chuck Lever