On Mon, Jun 30, 2008 at 06:58:14PM -0400, Chuck Lever wrote:
> Pass a more generic socket address type to nlmsvc_unlock_all_by_ip() to
> allow for future support of IPv6. Also provide additional sanity
> checking in failover_unlock_ip() when constructing the server's IP
> address.
>
> As an added bonus, provide clean kerneldoc comments on related NLM
> interfaces which were recently added.
Looks good to me, thanks--applied, with one minor change:
>
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>
> fs/lockd/svcsubs.c | 39 +++++++++++++++++++++++++--------------
> fs/nfsd/nfsctl.c | 15 ++++++++++-----
> include/linux/lockd/lockd.h | 2 +-
> 3 files changed, 36 insertions(+), 20 deletions(-)
>
>
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index d1c48b5..723b6d5 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -373,18 +373,18 @@ nlmsvc_free_host_resources(struct nlm_host *host)
> }
> }
>
> -/*
> - * Remove all locks held for clients
> +/**
> + * nlmsvc_invalidate_all - remove all locks held for clients
> + *
> + * Release all locks held by NFS clients. Previously, the code
> + * would call nlmsvc_free_host_resources for each client in turn,
> + * which is about as inefficient as it gets.
> + *
> + * Now we just do it once in nlm_traverse_files.
> */
> void
> nlmsvc_invalidate_all(void)
> {
> - /* Release all locks held by NFS clients.
> - * Previously, the code would call
> - * nlmsvc_free_host_resources for each client in
> - * turn, which is about as inefficient as it gets.
> - * Now we just do it once in nlm_traverse_files.
> - */
Mind if I keep the "Previously..." part in the body of the function?
I'd rather the kerneldoc comments be about the interface and leave
implementation details to the body of the function.
Also, one more minor question--
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 5ac00c4..5b4a412 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -310,9 +310,12 @@ static ssize_t write_getfd(struct file *file, char *buf, size_t size)
>
> static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
> {
> - __be32 server_ip;
> - char *fo_path, c;
> + struct sockaddr_in sin = {
> + .sin_family = AF_INET,
> + };
> int b1, b2, b3, b4;
> + char c;
> + char *fo_path;
>
> /* sanity check */
> if (size == 0)
> @@ -326,11 +329,13 @@ static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
> return -EINVAL;
>
> /* get ipv4 address */
> - if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
> + if (sscanf(fo_path, NIPQUAD_FMT "%c", &b1, &b2, &b3, &b4, &c) != 4)
> + return -EINVAL;
> + if (b1 > 255 || b2 > 255 || b3 > 255 || b4 > 255)
Should b1, b2, b3, b4 be unsigned?
--b.
> return -EINVAL;
> - server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
> + sin.sin_addr.s_addr = htonl((b1 << 24) | (b2 << 16) | (b3 << 8) | b4);
>
> - return nlmsvc_unlock_all_by_ip(server_ip);
> + return nlmsvc_unlock_all_by_ip((struct sockaddr *)&sin);
> }
On Tue, Jul 15, 2008 at 4:12 PM, J. Bruce Fields <[email protected]> wrote:
> On Mon, Jun 30, 2008 at 06:58:14PM -0400, Chuck Lever wrote:
>> Pass a more generic socket address type to nlmsvc_unlock_all_by_ip() to
>> allow for future support of IPv6. Also provide additional sanity
>> checking in failover_unlock_ip() when constructing the server's IP
>> address.
>>
>> As an added bonus, provide clean kerneldoc comments on related NLM
>> interfaces which were recently added.
>
> Looks good to me, thanks--applied, with one minor change:
>
>>
>> Signed-off-by: Chuck Lever <[email protected]>
>> ---
>>
>> fs/lockd/svcsubs.c | 39 +++++++++++++++++++++++++--------------
>> fs/nfsd/nfsctl.c | 15 ++++++++++-----
>> include/linux/lockd/lockd.h | 2 +-
>> 3 files changed, 36 insertions(+), 20 deletions(-)
>>
>>
>> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
>> index d1c48b5..723b6d5 100644
>> --- a/fs/lockd/svcsubs.c
>> +++ b/fs/lockd/svcsubs.c
>> @@ -373,18 +373,18 @@ nlmsvc_free_host_resources(struct nlm_host *host)
>> }
>> }
>>
>> -/*
>> - * Remove all locks held for clients
>> +/**
>> + * nlmsvc_invalidate_all - remove all locks held for clients
>> + *
>> + * Release all locks held by NFS clients. Previously, the code
>> + * would call nlmsvc_free_host_resources for each client in turn,
>> + * which is about as inefficient as it gets.
>> + *
>> + * Now we just do it once in nlm_traverse_files.
>> */
>> void
>> nlmsvc_invalidate_all(void)
>> {
>> - /* Release all locks held by NFS clients.
>> - * Previously, the code would call
>> - * nlmsvc_free_host_resources for each client in
>> - * turn, which is about as inefficient as it gets.
>> - * Now we just do it once in nlm_traverse_files.
>> - */
>
> Mind if I keep the "Previously..." part in the body of the function?
> I'd rather the kerneldoc comments be about the interface and leave
> implementation details to the body of the function.
It's a personal style choice. Keeping generic implementation comments
out of the body of the function makes cleaner code, I think, and makes
it more likely the author will write self-documenting code. In this
case, the comment is more historical than explanatory.
But I don't have any objection.
> Also, one more minor question--
>
>> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
>> index 5ac00c4..5b4a412 100644
>> --- a/fs/nfsd/nfsctl.c
>> +++ b/fs/nfsd/nfsctl.c
>> @@ -310,9 +310,12 @@ static ssize_t write_getfd(struct file *file, char *buf, size_t size)
>>
>> static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
>> {
>> - __be32 server_ip;
>> - char *fo_path, c;
>> + struct sockaddr_in sin = {
>> + .sin_family = AF_INET,
>> + };
>> int b1, b2, b3, b4;
>> + char c;
>> + char *fo_path;
>>
>> /* sanity check */
>> if (size == 0)
>> @@ -326,11 +329,13 @@ static ssize_t failover_unlock_ip(struct file *file, char *buf, size_t size)
>> return -EINVAL;
>>
>> /* get ipv4 address */
>> - if (sscanf(fo_path, "%u.%u.%u.%u%c", &b1, &b2, &b3, &b4, &c) != 4)
>> + if (sscanf(fo_path, NIPQUAD_FMT "%c", &b1, &b2, &b3, &b4, &c) != 4)
>> + return -EINVAL;
>> + if (b1 > 255 || b2 > 255 || b3 > 255 || b4 > 255)
>
> Should b1, b2, b3, b4 be unsigned?
That would be more consistent with the %u formatting, wouldn't it.
>> return -EINVAL;
>> - server_ip = htonl((((((b1<<8)|b2)<<8)|b3)<<8)|b4);
>> + sin.sin_addr.s_addr = htonl((b1 << 24) | (b2 << 16) | (b3 << 8) | b4);
>>
>> - return nlmsvc_unlock_all_by_ip(server_ip);
>> + return nlmsvc_unlock_all_by_ip((struct sockaddr *)&sin);
>> }
--
Chuck Lever