Re: [PATCH 05/12] cifs: rename cifs_strlcpy_to_host and make it use new functions

On Thu, 30 Apr 2009 14:35:15 +0530
Suresh Jayaraman <[email protected]> wrote:

> Jeff Layton wrote:
> > Rename cifs_strlcpy_to_host to cifs_strndup since that better describes
> > what this function really does. Then, convert it to use the new string
> > conversion and measurement functions that work in units of bytes rather
> > than wide chars.
> > 
> > Signed-off-by: Jeff Layton <[email protected]>
> > ---
> >  fs/cifs/cifs_unicode.c |   41 +++++++++++++++++++++++++++++++++++++++++
> >  fs/cifs/cifs_unicode.h |    2 ++
> >  fs/cifs/cifssmb.c      |   48 +++++++-----------------------------------------
> >  3 files changed, 50 insertions(+), 41 deletions(-)
> > 
> > diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> > index cd938bd..356f00c 100644
> > --- a/fs/cifs/cifs_unicode.c
> > +++ b/fs/cifs/cifs_unicode.c
> > @@ -245,3 +245,44 @@ cifs_strtoUCS(__le16 *to, const char *from, int len,
> >  	return i;
> >  }
> >  
> > +/*
> > + * cifs_strndup - copy a string from wire format to the local codepage
> > + * @dst - pointer to pointer of destination string
> > + * @src - source string
> > + * @maxlen - don't walk past this many bytes in the source string
> > + * @is_unicode - is this a unicode string?
> > + * @codepage - destination codepage
> > + *
> > + * Take a string given by the server, convert it to the local codepage and
> > + * put it in a new buffer. Returns length of the new buffer in bytes or a
> > + * negative error code. A pointer to the new buffer is placed into *dst.
> > + */
> > +int
> > +cifs_strndup(char **dst, const char *src, const int maxlen,
> > +	     const bool is_unicode, const struct nls_table *codepage)
> > +{
> > +	int len;
> > +
> > +	if (is_unicode) {
> > +		len = cifs_ucs2_bytes((__le16 *) src, maxlen, codepage);
> > +		len += nls_nullsize(codepage);
> > +		*dst = kmalloc(len, GFP_KERNEL);
> > +		if (!*dst)
> > +			goto err_exit;
> > +		cifs_from_ucs2(*dst, (__le16 *) src, len, maxlen, codepage,
> > +			       false);
> > +	} else {
> > +		len = strnlen(src, maxlen);
> > +		len++;
> > +		*dst = kmalloc(len, GFP_KERNEL);
> > +		if (!*dst)
> > +			goto err_exit;
> > +		strlcpy(*dst, src, len);
> > +	}
> > +	return len;
> 
> Do we really need to return len? Does any of the callers make use of
> this, now?
> 

We could just as easily make this function return a char pointer
instead and use PTR_ERR() to return errors. I had an earlier set that
did that and changed it at one point since I thought I needed the
length in one of the callers. It turns out that I don't. Returning a
char pointer here is probably a cleaner interface, so I'll make
that change in a bit.

> 
> > +
> > +err_exit:
> > +	cERROR(1, ("Failed to allocate buffer for string\n"));
> > +	return -ENOMEM;
> > +}
> > +
> > diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
> > index 1857f5f..9c7c9fc 100644
> > --- a/fs/cifs/cifs_unicode.h
> > +++ b/fs/cifs/cifs_unicode.h
> > @@ -78,6 +78,8 @@ int cifs_ucs2_bytes(const __le16 *from, int maxbytes,
> >  		    const struct nls_table *codepage);
> >  int cifs_strfromUCS_le(char *, const __le16 *, int, const struct nls_table *);
> >  int cifs_strtoUCS(__le16 *, const char *, int, const struct nls_table *);
> > +int cifs_strndup(char **dst, const char *src, const int maxlen,
> > +		 const bool is_unicode, const struct nls_table *codepage);
> >  #endif
> >  
> >  /*
> > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> > index cadacae..be90f22 100644
> > --- a/fs/cifs/cifssmb.c
> > +++ b/fs/cifs/cifssmb.c
> > @@ -81,41 +81,6 @@ static struct {
> >  #endif /* CONFIG_CIFS_WEAK_PW_HASH */
> >  #endif /* CIFS_POSIX */
> >  
> > -/* Allocates buffer into dst and copies smb string from src to it.
> > - * caller is responsible for freeing dst if function returned 0.
> > - * returns:
> > - * 	on success - 0
> > - *	on failure - errno
> > - */
> > -static int
> > -cifs_strlcpy_to_host(char **dst, const char *src, const int maxlen,
> > -		 const bool is_unicode, const struct nls_table *nls_codepage)
> > -{
> > -	int plen;
> > -
> > -	if (is_unicode) {
> > -		plen = UniStrnlen((wchar_t *)src, maxlen);
> > -		*dst = kmalloc((4 * plen) + 2, GFP_KERNEL);
> > -		if (!*dst)
> > -			goto cifs_strlcpy_to_host_ErrExit;
> > -		cifs_strfromUCS_le(*dst, (__le16 *)src, plen, nls_codepage);
> > -		(*dst)[plen] = 0;
> > -		(*dst)[plen+1] = 0; /* needed for Unicode */
> > -	} else {
> > -		plen = strnlen(src, maxlen);
> > -		*dst = kmalloc(plen + 2, GFP_KERNEL);
> > -		if (!*dst)
> > -			goto cifs_strlcpy_to_host_ErrExit;
> > -		strlcpy(*dst, src, plen);
> > -	}
> > -	return 0;
> > -
> > -cifs_strlcpy_to_host_ErrExit:
> > -	cERROR(1, ("Failed to allocate buffer for string\n"));
> > -	return -ENOMEM;
> > -}
> > -
> > -
> >  /* Mark as invalid, all open files on tree connections since they
> >     were closed when session to server was lost */
> >  static void mark_open_files_invalid(struct cifsTconInfo *pTcon)
> > @@ -4008,19 +3973,20 @@ parse_DFS_referrals(TRANSACTION2_GET_DFS_REFER_RSP *pSMBr,
> >  		/* copy DfsPath */
> >  		temp = (char *)ref + le16_to_cpu(ref->DfsPathOffset);
> >  		max_len = data_end - temp;
> > -		rc = cifs_strlcpy_to_host(&(node->path_name), temp,
> > -					max_len, is_unicode, nls_codepage);
> > -		if (rc)
> > +		rc = cifs_strndup(&node->path_name, temp, max_len,
> > +				  is_unicode, nls_codepage);
> > +		if (rc < 0)
> >  			goto parse_DFS_referrals_exit;
> >  
> >  		/* copy link target UNC */
> >  		temp = (char *)ref + le16_to_cpu(ref->NetworkAddressOffset);
> >  		max_len = data_end - temp;
> > -		rc = cifs_strlcpy_to_host(&(node->node_name), temp,
> > -					max_len, is_unicode, nls_codepage);
> > -		if (rc)
> > +		rc = cifs_strndup(&node->node_name, temp, max_len,
> > +				  is_unicode, nls_codepage);
> > +		if (rc < 0)
> >  			goto parse_DFS_referrals_exit;
> >  
> > +		rc = 0;
> >  		ref += le16_to_cpu(ref->Size);
> >  	}
> >  
> 
> 
> -- 
> Suresh Jayaraman


-- 
Jeff Layton <[email protected]>