diff mbox

sparse warning in cifsacl.c

Message ID BANLkTin-iQ7mrPifK-wM1BfM1HO6GsbjcQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Steve French May 27, 2011, 8:32 p.m. UTC
OK - after you and shirish take a look will send this trivial delta up
as a 2nd patch (the other is already merged).  I wish compiler were
smart enough to figure this out ... but Jeff's point makes sense.


On Fri, May 27, 2011 at 2:44 PM, Jeff Layton <jlayton@samba.org> wrote:
> On Thu, 26 May 2011 23:48:45 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> Shirish - below is a patch to fix a sparse warning in the cifsacl
>> code.  The change around line 458 is cosmetic (doesn't look like the
>> original code caused a problem but at least this quiets sparse) but
>> doesn't sid_authusers have an endian error.   How far back does this
>> bug go - does it affect much?
>>
>> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
>> index 076b69c..5f02b4e 100644
>> --- a/fs/cifs/cifsacl.c
>> +++ b/fs/cifs/cifsacl.c
>> @@ -38,7 +38,7 @@ static const struct cifs_sid sid_everyone = {
>>       1, 1, {0, 0, 0, 0, 0, 1}, {0} };
>>  /* security id for Authenticated Users system group */
>>  static const struct cifs_sid sid_authusers = {
>> -     1, 1, {0, 0, 0, 0, 0, 5}, {11} };
>> +     1, 1, {0, 0, 0, 0, 0, 5}, {cpu_to_le32(11)} };
>                                        ^^^^^^^^^
> This will break on big-endian arches. You need to use
> __constant_cpu_to_le32() instead.
>
>>  /* group users */
>>  static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
>>
>> @@ -458,7 +458,8 @@ int compare_sids(const struct cifs_sid *ctsid,
>> const struct cifs_sid *cwsid)
>>       if (num_subauth) {
>>               for (i = 0; i < num_subauth; ++i) {
>>                       if (ctsid->sub_auth[i] != cwsid->sub_auth[i]) {
>> -                             if (ctsid->sub_auth[i] > cwsid->sub_auth[i])
>> +                             if (le32_to_cpu(ctsid->sub_auth[i]) >
>> +                                     le32_to_cpu(cwsid->sub_auth[i]))
>>                                       return 1;
>>                               else
>>                                       return -1;
>>
>>
>
>
> --
> Jeff Layton <jlayton@samba.org>
>

Comments

Jeff Layton May 27, 2011, 8:34 p.m. UTC | #1
On Fri, 27 May 2011 15:32:11 -0500
Steve French <smfrench@gmail.com> wrote:

> OK - after you and shirish take a look will send this trivial delta up
> as a 2nd patch (the other is already merged).  I wish compiler were
> smart enough to figure this out ... but Jeff's point makes sense.
> 

The compiler does figure this out, but cpu_to_le32 is necessarily
arch-specific code, so it can't figure it out on a LE arch.

> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> index 5f02b4e..8f17006 100644
> --- a/fs/cifs/cifsacl.c
> +++ b/fs/cifs/cifsacl.c
> @@ -38,7 +38,7 @@ static const struct cifs_sid sid_everyone = {
>  	1, 1, {0, 0, 0, 0, 0, 1}, {0} };
>  /* security id for Authenticated Users system group */
>  static const struct cifs_sid sid_authusers = {
> -	1, 1, {0, 0, 0, 0, 0, 5}, {cpu_to_le32(11)} };
> +	1, 1, {0, 0, 0, 0, 0, 5}, {__constant_cpu_to_le32(11)} };
>  /* group users */
>  static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
> 

That looks better...

> 
> On Fri, May 27, 2011 at 2:44 PM, Jeff Layton <jlayton@samba.org> wrote:
> > On Thu, 26 May 2011 23:48:45 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >
> >> Shirish - below is a patch to fix a sparse warning in the cifsacl
> >> code.  The change around line 458 is cosmetic (doesn't look like the
> >> original code caused a problem but at least this quiets sparse) but
> >> doesn't sid_authusers have an endian error.   How far back does this
> >> bug go - does it affect much?
> >>
> >> diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
> >> index 076b69c..5f02b4e 100644
> >> --- a/fs/cifs/cifsacl.c
> >> +++ b/fs/cifs/cifsacl.c
> >> @@ -38,7 +38,7 @@ static const struct cifs_sid sid_everyone = {
> >>       1, 1, {0, 0, 0, 0, 0, 1}, {0} };
> >>  /* security id for Authenticated Users system group */
> >>  static const struct cifs_sid sid_authusers = {
> >> -     1, 1, {0, 0, 0, 0, 0, 5}, {11} };
> >> +     1, 1, {0, 0, 0, 0, 0, 5}, {cpu_to_le32(11)} };
> >                                        ^^^^^^^^^
> > This will break on big-endian arches. You need to use
> > __constant_cpu_to_le32() instead.
> >
> >>  /* group users */
> >>  static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };
> >>
> >> @@ -458,7 +458,8 @@ int compare_sids(const struct cifs_sid *ctsid,
> >> const struct cifs_sid *cwsid)
> >>       if (num_subauth) {
> >>               for (i = 0; i < num_subauth; ++i) {
> >>                       if (ctsid->sub_auth[i] != cwsid->sub_auth[i]) {
> >> -                             if (ctsid->sub_auth[i] > cwsid->sub_auth[i])
> >> +                             if (le32_to_cpu(ctsid->sub_auth[i]) >
> >> +                                     le32_to_cpu(cwsid->sub_auth[i]))
> >>                                       return 1;
> >>                               else
> >>                                       return -1;
> >>
> >>
> >
> >
> > --
> > Jeff Layton <jlayton@samba.org>
> >
> 
> 
>
diff mbox

Patch

diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c
index 5f02b4e..8f17006 100644
--- a/fs/cifs/cifsacl.c
+++ b/fs/cifs/cifsacl.c
@@ -38,7 +38,7 @@  static const struct cifs_sid sid_everyone = {
 	1, 1, {0, 0, 0, 0, 0, 1}, {0} };
 /* security id for Authenticated Users system group */
 static const struct cifs_sid sid_authusers = {
-	1, 1, {0, 0, 0, 0, 0, 5}, {cpu_to_le32(11)} };
+	1, 1, {0, 0, 0, 0, 0, 5}, {__constant_cpu_to_le32(11)} };
 /* group users */
 static const struct cifs_sid sid_user = {1, 2 , {0, 0, 0, 0, 0, 5}, {} };