[PATCH v2 00/10] gssd clean-ups for nfs-utils

[PATCH v2 00/10] gssd clean-ups for nfs-utils

[Chuck Lever]

I've been working on several fixes and clean-ups in response to an
issue reported by Veli-Matti Lintu <veli-matti.lintu@opinsys.fi>.
The thread is archived here:

 http://www.spinics.net/lists/linux-nfs/msg35306.html

There were two suggestions in that thread:

 1.  Clarify rpc.gssd(8)

 2.  Implement a gssd option to name a credential file where
     machine credentials can be found

This series implements the first suggestion along with a number of
clean-ups I found while working on this code.  The second suggestion
has been dropped for the moment.
      
I had this series with me at Connectathon, and did some light
testing there.  This series is ready to be merged into
nfs-utils.

Changes since v1:

  o  Acks from Bruce
  o  Restored part of the comment documenting
     gssd_find_existing_krb5_ccache()
  o  Dropped the patch adding the "-c" option to rpc.gssd.
     This issue will be revisited soon.

---

Chuck Lever (10):
      gssd: gethostname(3) returns zero or -1, not an errno
      gssd: Fix whitespace nits
      gssd: Clean up gssd_setup_krb5_user_gss_ccache()
      gssd: Update description of "-l" option
      gssd: Clarify use of the term "machine credentials" in rpc.gssd(8)
      gssd: Provide an introduction in gssd(8)
      gssd: gssd.man is missing a description of the "-M" option
      gssd: Use italics for option values and pathnames
      mountd: make local functions in v4root.c static
      mountd: remove unused variable


 utils/gssd/gssd.c      |    2 
 utils/gssd/gssd.man    |  310 +++++++++++++++++++++++++++++++++++-------------
 utils/gssd/gssd_proc.c |   12 +-
 utils/gssd/krb5_util.c |   20 ++-
 utils/mountd/cache.c   |    2 
 utils/mountd/v4root.c  |    6 +
 6 files changed, 244 insertions(+), 108 deletions(-)

-- 
Chuck Lever

[PATCH v2 08/10] gssd: Clean up gssd_setup_krb5_user_gss_ccache()

[Chuck Lever]

Remove a contradictory portion of the block comment documenting
gssd_find_existing_krb5_ccache().  This should have been removed by
commit 289ad31e, which reversed the meaning of the function's return
values.

Note that, in user space, typically errno's are positive.  But here
we follow the kernel convention of using negative values to return
error codes.  Make the documenting comments explicit about the sign
of an error return -- it will never be positive in the case of an
error.

And a nit: At the last return statement in
gssd_setup_krb5_user_gss_ccache(), "err" always contains zero, as
far as I can tell.  Make it explicit (to human readers) that when
execution reaches this point, gssd_setup_krb5_user_gss_ccache() is
going to return "success."

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Cc: Jeff Layton <jlayton@redhat.com>
---

 utils/gssd/krb5_util.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
index aeb8f70..940f7cd 100644
--- a/utils/gssd/krb5_util.c
+++ b/utils/gssd/krb5_util.c
@@ -169,13 +169,13 @@ select_krb5_ccache(const struct dirent *d)
 
 /*
  * Look in directory "dirname" for files that look like they
- * are Kerberos Credential Cache files for a given UID.  Return
- * non-zero and the dirent pointer for the entry most likely to be
- * what we want. Otherwise, return zero and no dirent pointer.
- * The caller is responsible for freeing the dirent if one is returned.
+ * are Kerberos Credential Cache files for a given UID.
  *
- * Returns 0 if a valid-looking entry was found and a non-zero error
- * code otherwise.
+ * Returns 0 if a valid-looking entry is found.  "*cctype" is
+ * set to the name of the cache type.  A pointer to the dirent
+ * is planted in "*d".  Caller must free "*d" with free(3).
+ *
+ * Otherwise, a negative errno is returned.
  */
 static int
 gssd_find_existing_krb5_ccache(uid_t uid, char *dirname,
@@ -1037,7 +1037,7 @@ err_cache:
  * given only a UID.  We really need more information, but we
  * do the best we can.
  *
- * Returns 0 if a ccache was found, and a non-zero error code otherwise.
+ * Returns 0 if a ccache was found, or a negative errno otherwise.
  */
 int
 gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
@@ -1082,7 +1082,7 @@ gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
 	printerr(2, "using %s as credentials cache for client with "
 		    "uid %u for server %s\n", buf, uid, servername);
 	gssd_set_krb5_ccache_name(buf);
-	return err;
+	return 0;
 }
 
 /*

Re: [PATCH v2 08/10] gssd: Clean up gssd_setup_krb5_user_gss_ccache()

[Jeff Layton]

On Tue, 12 Mar 2013 16:37:48 -0400
Chuck Lever <chuck.lever@oracle.com> wrote:

> Remove a contradictory portion of the block comment documenting
> gssd_find_existing_krb5_ccache().  This should have been removed by
> commit 289ad31e, which reversed the meaning of the function's return
> values.
> 
> Note that, in user space, typically errno's are positive.  But here
> we follow the kernel convention of using negative values to return
> error codes.  Make the documenting comments explicit about the sign
> of an error return -- it will never be positive in the case of an
> error.
> 
> And a nit: At the last return statement in
> gssd_setup_krb5_user_gss_ccache(), "err" always contains zero, as
> far as I can tell.  Make it explicit (to human readers) that when
> execution reaches this point, gssd_setup_krb5_user_gss_ccache() is
> going to return "success."
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> Cc: Jeff Layton <jlayton@redhat.com>
> ---
> 
>  utils/gssd/krb5_util.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/utils/gssd/krb5_util.c b/utils/gssd/krb5_util.c
> index aeb8f70..940f7cd 100644
> --- a/utils/gssd/krb5_util.c
> +++ b/utils/gssd/krb5_util.c
> @@ -169,13 +169,13 @@ select_krb5_ccache(const struct dirent *d)
>  
>  /*
>   * Look in directory "dirname" for files that look like they
> - * are Kerberos Credential Cache files for a given UID.  Return
> - * non-zero and the dirent pointer for the entry most likely to be
> - * what we want. Otherwise, return zero and no dirent pointer.
> - * The caller is responsible for freeing the dirent if one is returned.
> + * are Kerberos Credential Cache files for a given UID.
>   *
> - * Returns 0 if a valid-looking entry was found and a non-zero error
> - * code otherwise.
> + * Returns 0 if a valid-looking entry is found.  "*cctype" is
> + * set to the name of the cache type.  A pointer to the dirent
> + * is planted in "*d".  Caller must free "*d" with free(3).
> + *
> + * Otherwise, a negative errno is returned.
>   */
>  static int
>  gssd_find_existing_krb5_ccache(uid_t uid, char *dirname,
> @@ -1037,7 +1037,7 @@ err_cache:
>   * given only a UID.  We really need more information, but we
>   * do the best we can.
>   *
> - * Returns 0 if a ccache was found, and a non-zero error code otherwise.
> + * Returns 0 if a ccache was found, or a negative errno otherwise.
>   */
>  int
>  gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
> @@ -1082,7 +1082,7 @@ gssd_setup_krb5_user_gss_ccache(uid_t uid, char *servername, char *dirpattern)
>  	printerr(2, "using %s as credentials cache for client with "
>  		    "uid %u for server %s\n", buf, uid, servername);
>  	gssd_set_krb5_ccache_name(buf);
> -	return err;
> +	return 0;
>  }
>  
>  /*
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thanks for cleaning that up...

Reviewed-by: Jeff Layton <jlayton@redhat.com>

Re: [PATCH v2 00/10] gssd clean-ups for nfs-utils

[Steve Dickson]

On 12/03/13 16:36, Chuck Lever wrote:
> I've been working on several fixes and clean-ups in response to an
> issue reported by Veli-Matti Lintu <veli-matti.lintu@opinsys.fi>.
> The thread is archived here:
> 
>  http://www.spinics.net/lists/linux-nfs/msg35306.html
> 
> There were two suggestions in that thread:
> 
>  1.  Clarify rpc.gssd(8)
> 
>  2.  Implement a gssd option to name a credential file where
>      machine credentials can be found
> 
> This series implements the first suggestion along with a number of
> clean-ups I found while working on this code.  The second suggestion
> has been dropped for the moment.
>       
> I had this series with me at Connectathon, and did some light
> testing there.  This series is ready to be merged into
> nfs-utils.
> 
> Changes since v1:
> 
>   o  Acks from Bruce
>   o  Restored part of the comment documenting
>      gssd_find_existing_krb5_ccache()
>   o  Dropped the patch adding the "-c" option to rpc.gssd.
>      This issue will be revisited soon.
> 
> ---
> 
> Chuck Lever (10):
>       gssd: gethostname(3) returns zero or -1, not an errno
>       gssd: Fix whitespace nits
>       gssd: Clean up gssd_setup_krb5_user_gss_ccache()
>       gssd: Update description of "-l" option
>       gssd: Clarify use of the term "machine credentials" in rpc.gssd(8)
>       gssd: Provide an introduction in gssd(8)
>       gssd: gssd.man is missing a description of the "-M" option
>       gssd: Use italics for option values and pathnames
>       mountd: make local functions in v4root.c static
>       mountd: remove unused variable
> 
> 
>  utils/gssd/gssd.c      |    2 
>  utils/gssd/gssd.man    |  310 +++++++++++++++++++++++++++++++++++-------------
>  utils/gssd/gssd_proc.c |   12 +-
>  utils/gssd/krb5_util.c |   20 ++-
>  utils/mountd/cache.c   |    2 
>  utils/mountd/v4root.c  |    6 +
>  6 files changed, 244 insertions(+), 108 deletions(-)
> 
All Committed...

steved.