[OPW,kernel,02/10] staging: lustre: Use __FMODE_* macro instead of FMODE_*
diff mbox

Message ID 4f3fa35c7db2ca67b2e35bf03c34c606e35660ab.1383233124.git.dulshani.gunawardhana89@gmail.com
State New, archived
Headers show

Commit Message

Dulshani Gunawardhana Oct. 31, 2013, 3:31 p.m. UTC
Fix the following sparse warnings generated by AND-ing FMODE_* constant
with a normal integer by defining a new macro __FMODE_READ and
__FMODE_WRITE.

drivers/staging/lustre/lustre/llite/file.c:102:32: warning: restricted
fmode_t degrades to integer
drivers/staging/lustre/lustre/mdc/mdc_lib.c:179:47: warning: restricted
fmode_t degrades to integer

Signed-off-by: Dulshani Gunawardhana <dulshani.gunawardhana89@gmail.com>
---
 drivers/staging/lustre/lustre/include/lustre/lustre_idl.h | 5 +++++
 drivers/staging/lustre/lustre/llite/file.c                | 2 +-
 drivers/staging/lustre/lustre/mdc/mdc_lib.c               | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

Comments

Greg Kroah-Hartman Oct. 31, 2013, 3:45 p.m. UTC | #1
On Thu, Oct 31, 2013 at 09:01:18PM +0530, Dulshani Gunawardhana wrote:
> Fix the following sparse warnings generated by AND-ing FMODE_* constant
> with a normal integer by defining a new macro __FMODE_READ and
> __FMODE_WRITE.

I find it odd that a new macro has to be created here, and _really_
don't want to create a special one, just for a single driver.

How does the rest of the kernel deal with using these flags?  Why can't
that method be used here as well?

thanks,

greg k-h
Zach Brown Oct. 31, 2013, 5:24 p.m. UTC | #2
On Thu, Oct 31, 2013 at 08:45:41AM -0700, Greg KH wrote:
> On Thu, Oct 31, 2013 at 09:01:18PM +0530, Dulshani Gunawardhana wrote:
> > Fix the following sparse warnings generated by AND-ing FMODE_* constant
> > with a normal integer by defining a new macro __FMODE_READ and
> > __FMODE_WRITE.
> 
> I find it odd that a new macro has to be created here, and _really_
> don't want to create a special one, just for a single driver.
> 
> How does the rest of the kernel deal with using these flags?  Why can't
> that method be used here as well?

The kernel's FMODE flags use a sparse __bitwise__ type to keep users
from confusing them with the O_ userspace interface flags.

The lustre och_flags thing needs to be defined as fmode_t instead of int
if it's storing these FMODE flags.

- z
Dulshani Gunawardhana Nov. 2, 2013, 4:02 a.m. UTC | #3
Hi,


> The lustre och_flags thing needs to be defined as fmode_t instead of int 
> if it's storing these FMODE flags. 
>
>
This was actually my first approach. The problem I had was that the types 
of variables I changed were used for further ANDings with different types 
of variables.

The following is the code segment in mdc_lib.c.

static __u64 mds_pack_open_flags(__u32 flags, __u32 mode)

The first ANDing using FMODE_READ :

        __u64 cr_flags = (flags & (FMODE_READ | __FMODE_WRITE |
                                   MDS_OPEN_HAS_EA | MDS_OPEN_HAS_OBJS |
                                   MDS_OPEN_OWNEROVERRIDE | MDS_OPEN_LOCK |
                                   MDS_OPEN_BY_FID));

My problem is that afterwards 'flags' is in around five other ANDings with 
macros of type int type. 

if (flags & O_CREAT)
                cr_flags |= MDS_OPEN_CREAT;

So  changing the type of 'flags' gives even more warnings here.

I do understand now, after Greg's explanation, that declaring a new macro 
is definitely not the solution for this case. So I was thinking of 
(__force_int) to FMODE_*( ). Is this ok? Or would there be a better fix?

Thank you :).
Dulshani Gunawardhana Nov. 11, 2013, 10 a.m. UTC | #4
I think my question here was mis-looked over the sheer volume of the 
mailing list :) Anyway, as I'd like to tie up on the loose ends, would be 
grateful if a mentor could provide me guidance on how to solve this issue.


>> The lustre och_flags thing needs to be defined as fmode_t instead of int 
>> if it's storing these FMODE flags. 
>>
>>
> This was actually my first approach. The problem I had was that the types 
> of variables I changed were used for further ANDings with different types 
> of variables.
>
> The following is the code segment in mdc_lib.c.
>
> static __u64 mds_pack_open_flags(__u32 flags, __u32 mode)
>
> The first ANDing using FMODE_READ :
>
>         __u64 cr_flags = (flags & (FMODE_READ | __FMODE_WRITE |
>                                    MDS_OPEN_HAS_EA | MDS_OPEN_HAS_OBJS |
>                                    MDS_OPEN_OWNEROVERRIDE | MDS_OPEN_LOCK |
>                                    MDS_OPEN_BY_FID));
>
> My problem is that afterwards 'flags' is in around five other ANDings with 
> macros of type int type. 
>
> if (flags & O_CREAT)
>                 cr_flags |= MDS_OPEN_CREAT;
>
> So  changing the type of 'flags' gives even more warnings here.
>
>
What would be the appropriate fix in such a situation? Thanks :)
Greg Kroah-Hartman Nov. 11, 2013, 2:41 p.m. UTC | #5
On Mon, Nov 11, 2013 at 02:00:57AM -0800, Dulshani Gunawardhana wrote:
> I think my question here was mis-looked over the sheer volume of the mailing
> list :) Anyway, as I'd like to tie up on the loose ends, would be grateful if a
> mentor could provide me guidance on how to solve this issue.
> 
> 
> 
>         The lustre och_flags thing needs to be defined as fmode_t instead of
>         int
>         if it's storing these FMODE flags.
> 
> 
> 
>     This was actually my first approach. The problem I had was that the types
>     of variables I changed were used for further ANDings with different types
>     of variables.
> 
>     The following is the code segment in mdc_lib.c.
> 
>     static __u64 mds_pack_open_flags(__u32 flags, __u32 mode)
> 
>     The first ANDing using FMODE_READ :
> 
>             __u64 cr_flags = (flags & (FMODE_READ | __FMODE_WRITE |
>                                        MDS_OPEN_HAS_EA | MDS_OPEN_HAS_OBJS |
>                                        MDS_OPEN_OWNEROVERRIDE | MDS_OPEN_LOCK |
>                                        MDS_OPEN_BY_FID));
> 
>     My problem is that afterwards 'flags' is in around five other ANDings with
>     macros of type int type. 
> 
>     if (flags & O_CREAT)
>                     cr_flags |= MDS_OPEN_CREAT;
> 
>     So  changing the type of 'flags' gives even more warnings here.
> 
> 
> 
> What would be the appropriate fix in such a situation? Thanks :) 

Look and see how other parts of the kernel handle these bit fields.  At
the moment, I don't remember how it's done, but you shouldn't have to do
anything all that strange to deal with them properly.

good luck,

greg k-h

Patch
diff mbox

diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
index 5ca18d0..fe1bcbe 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
@@ -2337,6 +2337,11 @@  extern void lustre_swab_mdt_rec_setattr (struct mdt_rec_setattr *sa);
 #define FMODE_WRITE	      00000002
 #endif
 
+#ifndef __FMODE_READ
+#define __FMODE_READ        ((__force int) FMODE_READ)
+#define __FMODE_WRITE	    ((__force int) FMODE_WRITE)
+#endif
+
 #define MDS_FMODE_CLOSED	 00000000
 #define MDS_FMODE_EXEC	   00000004
 /* IO Epoch is opened on a closed file. */
diff --git a/drivers/staging/lustre/lustre/llite/file.c b/drivers/staging/lustre/lustre/llite/file.c
index fb85a58..c13d822 100644
--- a/drivers/staging/lustre/lustre/llite/file.c
+++ b/drivers/staging/lustre/lustre/llite/file.c
@@ -99,7 +99,7 @@  static void ll_prepare_close(struct inode *inode, struct md_op_data *op_data,
 					ATTR_MTIME | ATTR_MTIME_SET |
 					ATTR_CTIME | ATTR_CTIME_SET;
 
-	if (!(och->och_flags & FMODE_WRITE))
+	if (!(och->och_flags & __FMODE_WRITE))
 		goto out;
 
 	if (!exp_connect_som(ll_i2mdexp(inode)) || !S_ISREG(inode->i_mode))
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_lib.c b/drivers/staging/lustre/lustre/mdc/mdc_lib.c
index b2de478..29947fd 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_lib.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_lib.c
@@ -176,7 +176,7 @@  void mdc_create_pack(struct ptlrpc_request *req, struct md_op_data *op_data,
 
 static __u64 mds_pack_open_flags(__u32 flags, __u32 mode)
 {
-	__u64 cr_flags = (flags & (FMODE_READ | FMODE_WRITE |
+	__u64 cr_flags = (flags & (__FMODE_READ | __FMODE_WRITE |
 				   MDS_OPEN_HAS_EA | MDS_OPEN_HAS_OBJS |
 				   MDS_OPEN_OWNEROVERRIDE | MDS_OPEN_LOCK |
 				   MDS_OPEN_BY_FID));