cifs: Re-order M-F Symlink code

Discussion:

Dan Carpenter

2014-01-27 20:48:56 UTC

Hello Sachin Prabhu,

The patch 0f8dce1cb745: "cifs: Re-order M-F Symlink code" from Nov
25, 2013, leads to the following static checker warning:

fs/cifs/link.c:188 couldbe_mf_symlink()
error: should you be using something like S_ISREG() here?

fs/cifs/link.c
185 bool
186 couldbe_mf_symlink(const struct cifs_fattr *fattr)
187 {
188 if (!(fattr->cf_mode & S_IFREG))
^^^^^^
Should this be:

if (!S_ISREG(fattr->cf_mode))

189 /* it's not a symlink */
190 return false;
191
192 if (fattr->cf_eof != CIFS_MF_SYMLINK_FILE_SIZE)
193 /* it's not a symlink */
194 return false;
195
196 return true;
197 }

If this a real bug then please give the credit to Neil Brown because he
suggested I add this to Smatch (not pushed yet).

Reported-by: NeilBrown <neilb-***@public.gmane.org>

regards,
dan carpenter

Sachin Prabhu

2014-01-28 13:17:20 UTC

Permalink

Post by Dan Carpenter
Hello Sachin Prabhu,
The patch 0f8dce1cb745: "cifs: Re-order M-F Symlink code" from Nov
fs/cifs/link.c:188 couldbe_mf_symlink()
error: should you be using something like S_ISREG() here?
fs/cifs/link.c
185 bool
186 couldbe_mf_symlink(const struct cifs_fattr *fattr)
187 {
188 if (!(fattr->cf_mode & S_IFREG))
^^^^^^
if (!S_ISREG(fattr->cf_mode))
189 /* it's not a symlink */
190 return false;
191
192 if (fattr->cf_eof != CIFS_MF_SYMLINK_FILE_SIZE)
193 /* it's not a symlink */
194 return false;
195
196 return true;
197 }
If this a real bug then please give the credit to Neil Brown because he
suggested I add this to Smatch (not pushed yet).
regards,
dan carpenter

Hello Dan,

There seems to be no difference in operation between the original line
and the proposed line. The newer line just seems to be the preferred way
to test for regular files. Is there a bug I am missing here?

That particular line of code was simply copied over from the original
code it replaces.

You could propose a new patch for the change if you wish and I will be
happy to ack.

Sachin Prabhu

Dan Carpenter

2014-01-28 13:49:13 UTC

Permalink

Post by Sachin Prabhu

I mean, there is a functional difference in the obvious sense.

#define S_IFSOCK 0140000
#define S_IFLNK 0120000
#define S_IFREG 0100000
#define S_IFBLK 0060000
#define S_IFDIR 0040000
#define S_IFCHR 0020000
#define S_IFIFO 0010000
#define S_ISUID 0004000
#define S_ISGID 0002000
#define S_ISVTX 0001000

0100000 is set for S_IFREG, S_IFLNK and S_IFSOCK. But I don't know if
this makes a difference for cifs?

regards,
dan carpenter

Sachin Prabhu

2014-01-28 15:49:22 UTC

Permalink

Post by Dan Carpenter

Post by Sachin Prabhu

I mean, there is a functional difference in the obvious sense.
#define S_IFSOCK 0140000
#define S_IFLNK 0120000
#define S_IFREG 0100000
#define S_IFBLK 0060000
#define S_IFDIR 0040000
#define S_IFCHR 0020000
#define S_IFIFO 0010000
#define S_ISUID 0004000
#define S_ISGID 0002000
#define S_ISVTX 0001000
0100000 is set for S_IFREG, S_IFLNK and S_IFSOCK. But I don't know if
this makes a difference for cifs?

That is a good point. I will take a look at the code and send in a
patch.

Sachin Prabhu