[PATCH 0/5] cifs: various cleanups pointed out by David Howells...

[PATCH 0/5] cifs: various cleanups pointed out by David Howells...

[Jeff Layton]

David reviewed a set of cifs patches that I backported and pointed out
a few problems. This set is intended to fix them. Most are pretty
straightforward.

This should apply cleanly on top of the patch to switch the BCC to
little-endian. They compile cleanly without warnings and passed some
basic smoke testing with the connectathon tests.

Jeff Layton (5):
  cifs: turn BCC into a static inlined function
  cifs: remove unneeded variable initialization in cifs_reconnect_tcon
  cifs: fix comment in validate_t2
  cifs: clean up length checks in check2ndT2
  cifs: clean up various nits in unicode routines

 fs/cifs/cifs_unicode.c |   35 +++++++++++++++++------------------
 fs/cifs/cifs_unicode.h |    2 +-
 fs/cifs/cifspdu.h      |    9 ++++++---
 fs/cifs/cifssmb.c      |    7 ++++---
 fs/cifs/connect.c      |   26 +++++++++++++-------------
 5 files changed, 41 insertions(+), 38 deletions(-)

-- 
1.7.4

[PATCH 1/5] cifs: turn BCC into a static inlined function

[Jeff Layton]

It's a bad idea to have macro functions that reference variables more
than once, as the arguments could have side effects. Turn BCC() into
a static inlined function instead.

While we're at it, make it return a void * to discourage anyone from
dereferencing it as-is.

Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifspdu.h |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/cifs/cifspdu.h b/fs/cifs/cifspdu.h
index 291d735..de3aa28 100644
--- a/fs/cifs/cifspdu.h
+++ b/fs/cifs/cifspdu.h
@@ -428,9 +428,12 @@ struct smb_hdr {
 	__u8 WordCount;
 } __attribute__((packed));
 
-/* given a pointer to an smb_hdr retrieve a char pointer to the byte count */
-#define BCC(smb_var) ((unsigned char *)(smb_var) + sizeof(struct smb_hdr) + \
-			 (2 * (smb_var)->WordCount))
+/* given a pointer to an smb_hdr, retrieve a void pointer to the ByteCount */
+static inline void *
+BCC(struct smb_hdr *smb)
+{
+	return (void *)smb + sizeof(*smb) + 2 * smb->WordCount;
+}
 
 /* given a pointer to an smb_hdr retrieve the pointer to the byte area */
 #define pByteArea(smb_var) (BCC(smb_var) + 2)
-- 
1.7.4

[PATCH 2/5] cifs: remove unneeded variable initialization in cifs_reconnect_tcon

[Jeff Layton]

Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifssmb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 1622978..4f5c6d0 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -106,7 +106,7 @@ static void mark_open_files_invalid(struct cifs_tcon *pTcon)
 static int
 cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 {
-	int rc = 0;
+	int rc;
 	struct cifs_ses *ses;
 	struct TCP_Server_Info *server;
 	struct nls_table *nls_codepage;
-- 
1.7.4

Re: [PATCH 2/5] cifs: remove unneeded variable initialization in cifs_reconnect_tcon

[Steve French]

This is fine, but what about this alternative (initializing variables
can make code easier to read, and given dumb compiler warnings
sometimes better to leave in variable initialization ...):

stevef@stevef-laptop:~/cifs-2.6$ git diff -a
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 92e33d4..e4993a2 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
         * calling routine
         */
        if (!tcon)
-               return 0;
+               return rc;

        ses = tcon->ses;
        server = ses->server;


On Thu, Mar 31, 2011 at 4:36 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifssmb.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 1622978..4f5c6d0 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -106,7 +106,7 @@ static void mark_open_files_invalid(struct cifs_tcon *pTcon)
>  static int
>  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>  {
> -       int rc = 0;
> +       int rc;
>        struct cifs_ses *ses;
>        struct TCP_Server_Info *server;
>        struct nls_table *nls_codepage;
> --
> 1.7.4
>
>



-- 
Thanks,

Steve

Re: [PATCH 2/5] cifs: remove unneeded variable initialization in cifs_reconnect_tcon

[Jeff Layton]

On Tue, 5 Apr 2011 12:06:59 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> This is fine, but what about this alternative (initializing variables
> can make code easier to read, and given dumb compiler warnings
> sometimes better to leave in variable initialization ...):
> 
> stevef@stevef-laptop:~/cifs-2.6$ git diff -a
> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> index 92e33d4..e4993a2 100644
> --- a/fs/cifs/cifssmb.c
> +++ b/fs/cifs/cifssmb.c
> @@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>          * calling routine
>          */
>         if (!tcon)
> -               return 0;
> +               return rc;
> 
>         ses = tcon->ses;
>         server = ses->server;
> 
> 

So instead of eliminating a useless variable initialization, we instead
manufacture a use for it? That seems sort of counter-productive, no?

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 2/5] cifs: remove unneeded variable initialization in cifs_reconnect_tcon

[Steve French]

On Tue, Apr 5, 2011 at 12:17 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On Tue, 5 Apr 2011 12:06:59 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> This is fine, but what about this alternative (initializing variables
>> can make code easier to read, and given dumb compiler warnings
>> sometimes better to leave in variable initialization ...):
>>
>> stevef@stevef-laptop:~/cifs-2.6$ git diff -a
>> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
>> index 92e33d4..e4993a2 100644
>> --- a/fs/cifs/cifssmb.c
>> +++ b/fs/cifs/cifssmb.c
>> @@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
>>          * calling routine
>>          */
>>         if (!tcon)
>> -               return 0;
>> +               return rc;
>>
>>         ses = tcon->ses;
>>         server = ses->server;
>>
>>
>
> So instead of eliminating a useless variable initialization, we instead
> manufacture a use for it? That seems sort of counter-productive, no?

since we do "return rc" later in the function, why wouldn't we do the
same earlier
in the function for consistency? With rc initialized it is a bit easier to read.



-- 
Thanks,

Steve

Re: [PATCH 2/5] cifs: remove unneeded variable initialization in cifs_reconnect_tcon

[Jeff Layton]

On Tue, 5 Apr 2011 12:20:52 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Tue, Apr 5, 2011 at 12:17 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> > On Tue, 5 Apr 2011 12:06:59 -0500
> > Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> This is fine, but what about this alternative (initializing variables
> >> can make code easier to read, and given dumb compiler warnings
> >> sometimes better to leave in variable initialization ...):
> >>
> >> stevef@stevef-laptop:~/cifs-2.6$ git diff -a
> >> diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
> >> index 92e33d4..e4993a2 100644
> >> --- a/fs/cifs/cifssmb.c
> >> +++ b/fs/cifs/cifssmb.c
> >> @@ -117,7 +117,7 @@ cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
> >>          * calling routine
> >>          */
> >>         if (!tcon)
> >> -               return 0;
> >> +               return rc;
> >>
> >>         ses = tcon->ses;
> >>         server = ses->server;
> >>
> >>
> >
> > So instead of eliminating a useless variable initialization, we instead
> > manufacture a use for it? That seems sort of counter-productive, no?
> 
> since we do "return rc" later in the function, why wouldn't we do the
> same earlier
> in the function for consistency? With rc initialized it is a bit easier to read.
> 

In those places, we're returning rc because we called a function and
stored the return code from it in rc. You certainly could "return rc"
solely (and possibly use a "goto out" and have a single exit point for
the function). The above patch doesn't do that though, it only changes
the first spot where it does a "return 0".

If you want to make that change then you probably ought to change the
other places in the function for consistency. Again though, it seems
pointless to me. Leaving it unintialized may help the compiler catch
places where we "return rc" without setting it intentionally.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH 3/5] cifs: fix comment in validate_t2

[Jeff Layton]

The comment about checking the bcc is in the wrong place. Also make it
match kernel coding style.

Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifssmb.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 4f5c6d0..655f24c 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -339,12 +339,13 @@ static int validate_t2(struct smb_t2_rsp *pSMB)
 	    get_unaligned_le16(&pSMB->t2_rsp.DataOffset) > 1024)
 		goto vt2_err;
 
-	/* check that bcc is at least as big as parms + data */
-	/* check that bcc is less than negotiated smb buffer */
 	total_size = get_unaligned_le16(&pSMB->t2_rsp.ParameterCount);
 	if (total_size >= 512)
 		goto vt2_err;
 
+	/* check that bcc is at least as big as parms + data, and that it is
+	 * less than negotiated smb buffer
+	 */
 	total_size += get_unaligned_le16(&pSMB->t2_rsp.DataCount);
 	if (total_size > get_bcc(&pSMB->hdr) ||
 	    total_size >= CIFSMaxBufSize + MAX_CIFS_HDR_SIZE)
-- 
1.7.4

[PATCH 4/5] cifs: clean up length checks in check2ndT2

[Jeff Layton]

Thus spake David Howells:

The code that follows this:

  	remaining = total_data_size - data_in_this_rsp;
	if (remaining == 0)
		return 0;
	else if (remaining < 0) {

generates better code if you drop the 'remaining' variable and compare
the values directly.

Clean it up per his recommendation...

Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/connect.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 66d8de3..94e60c5 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -251,24 +251,24 @@ static int check2ndT2(struct smb_hdr *pSMB, unsigned int maxBufSize)
 	total_data_size = get_unaligned_le16(&pSMBt->t2_rsp.TotalDataCount);
 	data_in_this_rsp = get_unaligned_le16(&pSMBt->t2_rsp.DataCount);
 
-	remaining = total_data_size - data_in_this_rsp;
-
-	if (remaining == 0)
+	if (total_data_size == data_in_this_rsp)
 		return 0;
-	else if (remaining < 0) {
+	else if (total_data_size < data_in_this_rsp) {
 		cFYI(1, "total data %d smaller than data in frame %d",
 			total_data_size, data_in_this_rsp);
 		return -EINVAL;
-	} else {
-		cFYI(1, "missing %d bytes from transact2, check next response",
-			remaining);
-		if (total_data_size > maxBufSize) {
-			cERROR(1, "TotalDataSize %d is over maximum buffer %d",
-				total_data_size, maxBufSize);
-			return -EINVAL;
-		}
-		return remaining;
 	}
+
+	remaining = total_data_size - data_in_this_rsp;
+
+	cFYI(1, "missing %d bytes from transact2, check next response",
+		remaining);
+	if (total_data_size > maxBufSize) {
+		cERROR(1, "TotalDataSize %d is over maximum buffer %d",
+			total_data_size, maxBufSize);
+		return -EINVAL;
+	}
+	return remaining;
 }
 
 static int coalesce_t2(struct smb_hdr *psecond, struct smb_hdr *pTargetSMB)
-- 
1.7.4

[PATCH 5/5] cifs: clean up various nits in unicode routines

[Jeff Layton]

Fix the spelling of UNI_ASTERISK. Eliminate the unneeded len_remaining
variable in cifsConvertToUCS.

Also, as David Howells points out. We were better off making
cifsConvertToUCS *not* use put_unaligned_le16 since it means that we
can't optimize the mapped characters at compile time. Switch them
instead to use cpu_to_le16, and simply use put_unaligned to set them
in the string.

Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_unicode.c |   35 +++++++++++++++++------------------
 fs/cifs/cifs_unicode.h |    2 +-
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 9dd2682..e7ffade 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -90,7 +90,7 @@ cifs_mapchar(char *target, const __u16 src_char, const struct nls_table *cp,
 	case UNI_COLON:
 		*target = ':';
 		break;
-	case UNI_ASTERIK:
+	case UNI_ASTERISK:
 		*target = '*';
 		break;
 	case UNI_QUESTION:
@@ -264,40 +264,39 @@ cifs_strndup_from_ucs(const char *src, const int maxlen, const bool is_unicode,
  * names are little endian 16 bit Unicode on the wire
  */
 int
-cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
+cifsConvertToUCS(__le16 *target, const char *source, int srclen,
 		 const struct nls_table *cp, int mapChars)
 {
 	int i, j, charlen;
-	int len_remaining = maxlen;
 	char src_char;
-	__u16 temp;
+	__le16 temp;
 
 	if (!mapChars)
 		return cifs_strtoUCS(target, source, PATH_MAX, cp);
 
-	for (i = 0, j = 0; i < maxlen; j++) {
+	for (i = 0, j = 0; i < srclen; j++) {
 		src_char = source[i];
 		switch (src_char) {
 		case 0:
-			put_unaligned_le16(0, &target[j]);
+			put_unaligned(0, &target[j]);
 			goto ctoUCS_out;
 		case ':':
-			temp = UNI_COLON;
+			temp = cpu_to_le16(UNI_COLON);
 			break;
 		case '*':
-			temp = UNI_ASTERIK;
+			temp = cpu_to_le16(UNI_ASTERISK);
 			break;
 		case '?':
-			temp = UNI_QUESTION;
+			temp = cpu_to_le16(UNI_QUESTION);
 			break;
 		case '<':
-			temp = UNI_LESSTHAN;
+			temp = cpu_to_le16(UNI_LESSTHAN);
 			break;
 		case '>':
-			temp = UNI_GRTRTHAN;
+			temp = cpu_to_le16(UNI_GRTRTHAN);
 			break;
 		case '|':
-			temp = UNI_PIPE;
+			temp = cpu_to_le16(UNI_PIPE);
 			break;
 		/*
 		 * FIXME: We can not handle remapping backslash (UNI_SLASH)
@@ -305,17 +304,18 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
 		 * as they use backslash as separator.
 		 */
 		default:
-			charlen = cp->char2uni(source+i, len_remaining,
-						&temp);
+			charlen = cp->char2uni(source + i, srclen - i,
+						(wchar_t *)&temp);
+			temp = cpu_to_le16(temp);
+
 			/*
 			 * if no match, use question mark, which at least in
 			 * some cases serves as wild card
 			 */
 			if (charlen < 1) {
-				temp = 0x003f;
+				temp = cpu_to_le16(0x003f);
 				charlen = 1;
 			}
-			len_remaining -= charlen;
 			/*
 			 * character may take more than one byte in the source
 			 * string, but will take exactly two bytes in the
@@ -324,9 +324,8 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
 			i += charlen;
 			continue;
 		}
-		put_unaligned_le16(temp, &target[j]);
+		put_unaligned(temp, &target[j]);
 		i++; /* move to next char in source string */
-		len_remaining--;
 	}
 
 ctoUCS_out:
diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
index cc9628b..e00f677 100644
--- a/fs/cifs/cifs_unicode.h
+++ b/fs/cifs/cifs_unicode.h
@@ -44,7 +44,7 @@
  * reserved symbols (along with \ and /), otherwise illegal to store
  * in filenames in NTFS
  */
-#define UNI_ASTERIK     (__u16) ('*' + 0xF000)
+#define UNI_ASTERISK    (__u16) ('*' + 0xF000)
 #define UNI_QUESTION    (__u16) ('?' + 0xF000)
 #define UNI_COLON       (__u16) (':' + 0xF000)
 #define UNI_GRTRTHAN    (__u16) ('>' + 0xF000)
-- 
1.7.4

Re: [PATCH 5/5] cifs: clean up various nits in unicode routines

[Steve French]

The patch is fine except that I would prefer that it not introduce a
sparse warning
(this would be the only endian warning). See below:

  CHECK   fs/cifs/cifs_unicode.c
fs/cifs/cifs_unicode.c:309:32: warning: cast from restricted __le16
  CC [M]  fs/cifs/cifs_unicode.o


On Thu, Mar 31, 2011 at 4:36 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Fix the spelling of UNI_ASTERISK. Eliminate the unneeded len_remaining
> variable in cifsConvertToUCS.
>
> Also, as David Howells points out. We were better off making
> cifsConvertToUCS *not* use put_unaligned_le16 since it means that we
> can't optimize the mapped characters at compile time. Switch them
> instead to use cpu_to_le16, and simply use put_unaligned to set them
> in the string.
>
> Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifs_unicode.c |   35 +++++++++++++++++------------------
>  fs/cifs/cifs_unicode.h |    2 +-
>  2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 9dd2682..e7ffade 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -90,7 +90,7 @@ cifs_mapchar(char *target, const __u16 src_char, const struct nls_table *cp,
>        case UNI_COLON:
>                *target = ':';
>                break;
> -       case UNI_ASTERIK:
> +       case UNI_ASTERISK:
>                *target = '*';
>                break;
>        case UNI_QUESTION:
> @@ -264,40 +264,39 @@ cifs_strndup_from_ucs(const char *src, const int maxlen, const bool is_unicode,
>  * names are little endian 16 bit Unicode on the wire
>  */
>  int
> -cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
> +cifsConvertToUCS(__le16 *target, const char *source, int srclen,
>                 const struct nls_table *cp, int mapChars)
>  {
>        int i, j, charlen;
> -       int len_remaining = maxlen;
>        char src_char;
> -       __u16 temp;
> +       __le16 temp;
>
>        if (!mapChars)
>                return cifs_strtoUCS(target, source, PATH_MAX, cp);
>
> -       for (i = 0, j = 0; i < maxlen; j++) {
> +       for (i = 0, j = 0; i < srclen; j++) {
>                src_char = source[i];
>                switch (src_char) {
>                case 0:
> -                       put_unaligned_le16(0, &target[j]);
> +                       put_unaligned(0, &target[j]);
>                        goto ctoUCS_out;
>                case ':':
> -                       temp = UNI_COLON;
> +                       temp = cpu_to_le16(UNI_COLON);
>                        break;
>                case '*':
> -                       temp = UNI_ASTERIK;
> +                       temp = cpu_to_le16(UNI_ASTERISK);
>                        break;
>                case '?':
> -                       temp = UNI_QUESTION;
> +                       temp = cpu_to_le16(UNI_QUESTION);
>                        break;
>                case '<':
> -                       temp = UNI_LESSTHAN;
> +                       temp = cpu_to_le16(UNI_LESSTHAN);
>                        break;
>                case '>':
> -                       temp = UNI_GRTRTHAN;
> +                       temp = cpu_to_le16(UNI_GRTRTHAN);
>                        break;
>                case '|':
> -                       temp = UNI_PIPE;
> +                       temp = cpu_to_le16(UNI_PIPE);
>                        break;
>                /*
>                 * FIXME: We can not handle remapping backslash (UNI_SLASH)
> @@ -305,17 +304,18 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
>                 * as they use backslash as separator.
>                 */
>                default:
> -                       charlen = cp->char2uni(source+i, len_remaining,
> -                                               &temp);
> +                       charlen = cp->char2uni(source + i, srclen - i,
> +                                               (wchar_t *)&temp);
> +                       temp = cpu_to_le16(temp);
> +
>                        /*
>                         * if no match, use question mark, which at least in
>                         * some cases serves as wild card
>                         */
>                        if (charlen < 1) {
> -                               temp = 0x003f;
> +                               temp = cpu_to_le16(0x003f);
>                                charlen = 1;
>                        }
> -                       len_remaining -= charlen;
>                        /*
>                         * character may take more than one byte in the source
>                         * string, but will take exactly two bytes in the
> @@ -324,9 +324,8 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
>                        i += charlen;
>                        continue;
>                }
> -               put_unaligned_le16(temp, &target[j]);
> +               put_unaligned(temp, &target[j]);
>                i++; /* move to next char in source string */
> -               len_remaining--;
>        }
>
>  ctoUCS_out:
> diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
> index cc9628b..e00f677 100644
> --- a/fs/cifs/cifs_unicode.h
> +++ b/fs/cifs/cifs_unicode.h
> @@ -44,7 +44,7 @@
>  * reserved symbols (along with \ and /), otherwise illegal to store
>  * in filenames in NTFS
>  */
> -#define UNI_ASTERIK     (__u16) ('*' + 0xF000)
> +#define UNI_ASTERISK    (__u16) ('*' + 0xF000)
>  #define UNI_QUESTION    (__u16) ('?' + 0xF000)
>  #define UNI_COLON       (__u16) (':' + 0xF000)
>  #define UNI_GRTRTHAN    (__u16) ('>' + 0xF000)
> --
> 1.7.4
>
>



-- 
Thanks,

Steve

Re: [PATCH 5/5] cifs: clean up various nits in unicode routines

[David Howells]

Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> On Tue, 5 Apr 2011 12:23:40 -0500
> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > The patch is fine except that I would prefer that it not introduce a
> > sparse warning
> > (this would be the only endian warning). See below:
> > 
> >   CHECK   fs/cifs/cifs_unicode.c
> > fs/cifs/cifs_unicode.c:309:32: warning: cast from restricted __le16
> >   CC [M]  fs/cifs/cifs_unicode.o
> > 
> 
> Good point. I'll need to put a new variable on the stack to fix it, but
> that's no big deal. I'll fix and resend.

Which line is it that actually gives the warning?  Is it when 0 is implicitly
cast to __le16?  If so, can you just cast it directly?

David

Re: [PATCH 5/5] cifs: clean up various nits in unicode routines

[Steve French]

+                       charlen = cp->char2uni(source + i, srclen - i,
+                                               (wchar_t *)&temp);
+                       temp = cpu_to_le16(temp);


On Tue, Apr 5, 2011 at 12:58 PM, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
>> On Tue, 5 Apr 2011 12:23:40 -0500
>> Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>
>> > The patch is fine except that I would prefer that it not introduce a
>> > sparse warning
>> > (this would be the only endian warning). See below:
>> >
>> >   CHECK   fs/cifs/cifs_unicode.c
>> > fs/cifs/cifs_unicode.c:309:32: warning: cast from restricted __le16
>> >   CC [M]  fs/cifs/cifs_unicode.o
>> >
>>
>> Good point. I'll need to put a new variable on the stack to fix it, but
>> that's no big deal. I'll fix and resend.
>
> Which line is it that actually gives the warning?  Is it when 0 is implicitly
> cast to __le16?  If so, can you just cast it directly?
>
> David
>



-- 
Thanks,

Steve

Re: [PATCH 5/5] cifs: clean up various nits in unicode routines

[David Howells]

David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> Which line is it that actually gives the warning?  Is it when 0 is implicitly
> cast to __le16?  If so, can you just cast it directly?

Actually, if it really *is* that line, then perhaps *sparse* is wrong... (0 is
special after all).

David

Re: [PATCH 5/5] cifs: clean up various nits in unicode routines

[Jeff Layton]

On Tue, 05 Apr 2011 19:01:18 +0100
David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:

> David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > Which line is it that actually gives the warning?  Is it when 0 is implicitly
> > cast to __le16?  If so, can you just cast it directly?
> 
> Actually, if it really *is* that line, then perhaps *sparse* is wrong... (0 is
> special after all).
> 
> David

No, it's because I'm abusing the __le16 on the stack and then fixing it
up using cpu_to_le16. It's worth fixing, I think...

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH 5/5] cifs: clean up various nits in unicode routines

[Jeff Layton]

On Tue, 5 Apr 2011 12:23:40 -0500
Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> The patch is fine except that I would prefer that it not introduce a
> sparse warning
> (this would be the only endian warning). See below:
> 
>   CHECK   fs/cifs/cifs_unicode.c
> fs/cifs/cifs_unicode.c:309:32: warning: cast from restricted __le16
>   CC [M]  fs/cifs/cifs_unicode.o
> 

Good point. I'll need to put a new variable on the stack to fix it, but
that's no big deal. I'll fix and resend.
-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[PATCH] cifs: clean up various nits in unicode routines (try #2)

[Jeff Layton]

Minor revision to the original patch. Don't abuse the __le16 variable
on the stack by casting it to wchar_t and handing it off to char2uni.
Declare an actual wchar_t on the stack instead. This fixes a valid
sparse warning.

Fix the spelling of UNI_ASTERISK. Eliminate the unneeded len_remaining
variable in cifsConvertToUCS.

Also, as David Howells points out. We were better off making
cifsConvertToUCS *not* use put_unaligned_le16 since it means that we
can't optimize the mapped characters at compile time. Switch them
instead to use cpu_to_le16, and simply use put_unaligned to set them
in the string.

Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/cifs/cifs_unicode.c |   35 +++++++++++++++++------------------
 fs/cifs/cifs_unicode.h |    2 +-
 2 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
index 9dd2682..d8e93b0 100644
--- a/fs/cifs/cifs_unicode.c
+++ b/fs/cifs/cifs_unicode.c
@@ -90,7 +90,7 @@ cifs_mapchar(char *target, const __u16 src_char, const struct nls_table *cp,
 	case UNI_COLON:
 		*target = ':';
 		break;
-	case UNI_ASTERIK:
+	case UNI_ASTERISK:
 		*target = '*';
 		break;
 	case UNI_QUESTION:
@@ -264,40 +264,40 @@ cifs_strndup_from_ucs(const char *src, const int maxlen, const bool is_unicode,
  * names are little endian 16 bit Unicode on the wire
  */
 int
-cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
+cifsConvertToUCS(__le16 *target, const char *source, int srclen,
 		 const struct nls_table *cp, int mapChars)
 {
 	int i, j, charlen;
-	int len_remaining = maxlen;
 	char src_char;
-	__u16 temp;
+	__le16 dst_char;
+	wchar_t tmp;
 
 	if (!mapChars)
 		return cifs_strtoUCS(target, source, PATH_MAX, cp);
 
-	for (i = 0, j = 0; i < maxlen; j++) {
+	for (i = 0, j = 0; i < srclen; j++) {
 		src_char = source[i];
 		switch (src_char) {
 		case 0:
-			put_unaligned_le16(0, &target[j]);
+			put_unaligned(0, &target[j]);
 			goto ctoUCS_out;
 		case ':':
-			temp = UNI_COLON;
+			dst_char = cpu_to_le16(UNI_COLON);
 			break;
 		case '*':
-			temp = UNI_ASTERIK;
+			dst_char = cpu_to_le16(UNI_ASTERISK);
 			break;
 		case '?':
-			temp = UNI_QUESTION;
+			dst_char = cpu_to_le16(UNI_QUESTION);
 			break;
 		case '<':
-			temp = UNI_LESSTHAN;
+			dst_char = cpu_to_le16(UNI_LESSTHAN);
 			break;
 		case '>':
-			temp = UNI_GRTRTHAN;
+			dst_char = cpu_to_le16(UNI_GRTRTHAN);
 			break;
 		case '|':
-			temp = UNI_PIPE;
+			dst_char = cpu_to_le16(UNI_PIPE);
 			break;
 		/*
 		 * FIXME: We can not handle remapping backslash (UNI_SLASH)
@@ -305,17 +305,17 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
 		 * as they use backslash as separator.
 		 */
 		default:
-			charlen = cp->char2uni(source+i, len_remaining,
-						&temp);
+			charlen = cp->char2uni(source + i, srclen - i, &tmp);
+			dst_char = cpu_to_le16(tmp);
+
 			/*
 			 * if no match, use question mark, which at least in
 			 * some cases serves as wild card
 			 */
 			if (charlen < 1) {
-				temp = 0x003f;
+				dst_char = cpu_to_le16(0x003f);
 				charlen = 1;
 			}
-			len_remaining -= charlen;
 			/*
 			 * character may take more than one byte in the source
 			 * string, but will take exactly two bytes in the
@@ -324,9 +324,8 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
 			i += charlen;
 			continue;
 		}
-		put_unaligned_le16(temp, &target[j]);
+		put_unaligned(dst_char, &target[j]);
 		i++; /* move to next char in source string */
-		len_remaining--;
 	}
 
 ctoUCS_out:
diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
index cc9628b..e00f677 100644
--- a/fs/cifs/cifs_unicode.h
+++ b/fs/cifs/cifs_unicode.h
@@ -44,7 +44,7 @@
  * reserved symbols (along with \ and /), otherwise illegal to store
  * in filenames in NTFS
  */
-#define UNI_ASTERIK     (__u16) ('*' + 0xF000)
+#define UNI_ASTERISK    (__u16) ('*' + 0xF000)
 #define UNI_QUESTION    (__u16) ('?' + 0xF000)
 #define UNI_COLON       (__u16) (':' + 0xF000)
 #define UNI_GRTRTHAN    (__u16) ('>' + 0xF000)
-- 
1.7.4

Re: [PATCH] cifs: clean up various nits in unicode routines (try #2)

[Steve French]

merged into cifs-2.6.git

On Tue, Apr 5, 2011 at 2:02 PM, Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Minor revision to the original patch. Don't abuse the __le16 variable
> on the stack by casting it to wchar_t and handing it off to char2uni.
> Declare an actual wchar_t on the stack instead. This fixes a valid
> sparse warning.
>
> Fix the spelling of UNI_ASTERISK. Eliminate the unneeded len_remaining
> variable in cifsConvertToUCS.
>
> Also, as David Howells points out. We were better off making
> cifsConvertToUCS *not* use put_unaligned_le16 since it means that we
> can't optimize the mapped characters at compile time. Switch them
> instead to use cpu_to_le16, and simply use put_unaligned to set them
> in the string.
>
> Reported-and-acked-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/cifs/cifs_unicode.c |   35 +++++++++++++++++------------------
>  fs/cifs/cifs_unicode.h |    2 +-
>  2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/fs/cifs/cifs_unicode.c b/fs/cifs/cifs_unicode.c
> index 9dd2682..d8e93b0 100644
> --- a/fs/cifs/cifs_unicode.c
> +++ b/fs/cifs/cifs_unicode.c
> @@ -90,7 +90,7 @@ cifs_mapchar(char *target, const __u16 src_char, const struct nls_table *cp,
>        case UNI_COLON:
>                *target = ':';
>                break;
> -       case UNI_ASTERIK:
> +       case UNI_ASTERISK:
>                *target = '*';
>                break;
>        case UNI_QUESTION:
> @@ -264,40 +264,40 @@ cifs_strndup_from_ucs(const char *src, const int maxlen, const bool is_unicode,
>  * names are little endian 16 bit Unicode on the wire
>  */
>  int
> -cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
> +cifsConvertToUCS(__le16 *target, const char *source, int srclen,
>                 const struct nls_table *cp, int mapChars)
>  {
>        int i, j, charlen;
> -       int len_remaining = maxlen;
>        char src_char;
> -       __u16 temp;
> +       __le16 dst_char;
> +       wchar_t tmp;
>
>        if (!mapChars)
>                return cifs_strtoUCS(target, source, PATH_MAX, cp);
>
> -       for (i = 0, j = 0; i < maxlen; j++) {
> +       for (i = 0, j = 0; i < srclen; j++) {
>                src_char = source[i];
>                switch (src_char) {
>                case 0:
> -                       put_unaligned_le16(0, &target[j]);
> +                       put_unaligned(0, &target[j]);
>                        goto ctoUCS_out;
>                case ':':
> -                       temp = UNI_COLON;
> +                       dst_char = cpu_to_le16(UNI_COLON);
>                        break;
>                case '*':
> -                       temp = UNI_ASTERIK;
> +                       dst_char = cpu_to_le16(UNI_ASTERISK);
>                        break;
>                case '?':
> -                       temp = UNI_QUESTION;
> +                       dst_char = cpu_to_le16(UNI_QUESTION);
>                        break;
>                case '<':
> -                       temp = UNI_LESSTHAN;
> +                       dst_char = cpu_to_le16(UNI_LESSTHAN);
>                        break;
>                case '>':
> -                       temp = UNI_GRTRTHAN;
> +                       dst_char = cpu_to_le16(UNI_GRTRTHAN);
>                        break;
>                case '|':
> -                       temp = UNI_PIPE;
> +                       dst_char = cpu_to_le16(UNI_PIPE);
>                        break;
>                /*
>                 * FIXME: We can not handle remapping backslash (UNI_SLASH)
> @@ -305,17 +305,17 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
>                 * as they use backslash as separator.
>                 */
>                default:
> -                       charlen = cp->char2uni(source+i, len_remaining,
> -                                               &temp);
> +                       charlen = cp->char2uni(source + i, srclen - i, &tmp);
> +                       dst_char = cpu_to_le16(tmp);
> +
>                        /*
>                         * if no match, use question mark, which at least in
>                         * some cases serves as wild card
>                         */
>                        if (charlen < 1) {
> -                               temp = 0x003f;
> +                               dst_char = cpu_to_le16(0x003f);
>                                charlen = 1;
>                        }
> -                       len_remaining -= charlen;
>                        /*
>                         * character may take more than one byte in the source
>                         * string, but will take exactly two bytes in the
> @@ -324,9 +324,8 @@ cifsConvertToUCS(__le16 *target, const char *source, int maxlen,
>                        i += charlen;
>                        continue;
>                }
> -               put_unaligned_le16(temp, &target[j]);
> +               put_unaligned(dst_char, &target[j]);
>                i++; /* move to next char in source string */
> -               len_remaining--;
>        }
>
>  ctoUCS_out:
> diff --git a/fs/cifs/cifs_unicode.h b/fs/cifs/cifs_unicode.h
> index cc9628b..e00f677 100644
> --- a/fs/cifs/cifs_unicode.h
> +++ b/fs/cifs/cifs_unicode.h
> @@ -44,7 +44,7 @@
>  * reserved symbols (along with \ and /), otherwise illegal to store
>  * in filenames in NTFS
>  */
> -#define UNI_ASTERIK     (__u16) ('*' + 0xF000)
> +#define UNI_ASTERISK    (__u16) ('*' + 0xF000)
>  #define UNI_QUESTION    (__u16) ('?' + 0xF000)
>  #define UNI_COLON       (__u16) (':' + 0xF000)
>  #define UNI_GRTRTHAN    (__u16) ('>' + 0xF000)
> --
> 1.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Thanks,

Steve