| Message ID | 34dc78b262546e9343e0ed872232a97f5eaa5f15.1426502566.git.osandov@osandov.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Mon, Mar 16, 2015 at 04:33:49AM -0700, Omar Sandoval wrote: > Get either READ or WRITE out of iter->type. Umm... > + * Get one of READ or WRITE out of iter->type without any other flags OR'd in > + * with it. > + */ > +static inline int iov_iter_rw(const struct iov_iter *i) > +{ > + return i->type & RW_MASK; > +} TBH, I would turn that into a macro. Reason: indirect includes. How about #define iov_iter_rw(i) ((0 ? (struct iov_iter *)0 : (i))->type & RW_MASK) Should do you all the type safety of inline function and avoids the need to include fs.h in uio.h; _users_ of iov_iter_rw() obviously still need fs.h, but such places always used to... -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 16, 2015 at 05:36:05PM +0000, Al Viro wrote: > On Mon, Mar 16, 2015 at 04:33:49AM -0700, Omar Sandoval wrote: > > Get either READ or WRITE out of iter->type. > > Umm... > > > + * Get one of READ or WRITE out of iter->type without any other flags OR'd in > > + * with it. > > + */ > > +static inline int iov_iter_rw(const struct iov_iter *i) > > +{ > > + return i->type & RW_MASK; > > +} > > TBH, I would turn that into a macro. Reason: indirect includes. Agreed, but the proposed define is rather cryptic and I was not able to understand the meaning on the first glance. > #define iov_iter_rw(i) ((0 ? (struct iov_iter *)0 : (i))->type & RW_MASK) This worked for me, does not compile with anything else than 'struct iov_iter*' as i: #define iov_iter_rw(i) ({ \ struct iov_iter __iter = *(i); \ (i)->type & RW_MASK; \ }) The assignment is optimized out. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 17, 2015 at 10:31:51AM +0100, David Sterba wrote: > On Mon, Mar 16, 2015 at 05:36:05PM +0000, Al Viro wrote: > > On Mon, Mar 16, 2015 at 04:33:49AM -0700, Omar Sandoval wrote: > > > Get either READ or WRITE out of iter->type. > > > > Umm... > > > > > + * Get one of READ or WRITE out of iter->type without any other flags OR'd in > > > + * with it. > > > + */ > > > +static inline int iov_iter_rw(const struct iov_iter *i) > > > +{ > > > + return i->type & RW_MASK; > > > +} > > > > TBH, I would turn that into a macro. Reason: indirect includes. > > Agreed, but the proposed define is rather cryptic and I was not able to > understand the meaning on the first glance. > > > #define iov_iter_rw(i) ((0 ? (struct iov_iter *)0 : (i))->type & RW_MASK) > > This worked for me, does not compile with anything else than > 'struct iov_iter*' as i: > > #define iov_iter_rw(i) ({ \ > struct iov_iter __iter = *(i); \ > (i)->type & RW_MASK; \ > }) > > The assignment is optimized out. [-cc individual fs maintainers to avoid all of these email bounces, should've looked a bit closer at that get_maintainer.pl output...] I agree that this is a bit more readable, but it evaluates i twice. That's an easy fix, just do __iter.type instead of (i)->type, but there's still the possibility of someone passing in something called __iter as i, and the fix for that tends to be "add more underscores". At the very least, Al's macro could probably use a comment explaining what's going on there, though.
On Tue, Mar 17, 2015 at 10:31:51AM +0100, David Sterba wrote: > Agreed, but the proposed define is rather cryptic and I was not able to > understand the meaning on the first glance. > > > #define iov_iter_rw(i) ((0 ? (struct iov_iter *)0 : (i))->type & RW_MASK) > > This worked for me, does not compile with anything else than > 'struct iov_iter*' as i: > > #define iov_iter_rw(i) ({ \ > struct iov_iter __iter = *(i); \ > (i)->type & RW_MASK; \ > }) > > The assignment is optimized out. ... and you are getting a) use of rather lousy gccism when plain C would do b) double evaluation since you've got it wrong (should've been __iter.type & RW_MASK, if you do it that way). As it is, if argument has any side effects, your variant will trigger those twice - even if the destination of the assignment is never used, the side effects remain. I agree that it could use /* use ?: for typechecking */, but let's not go into ({...}) land unless we absolutely have to. -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/uio.h b/include/linux/uio.h index 7188029..87a47b3 100644 --- a/include/linux/uio.h +++ b/include/linux/uio.h @@ -10,6 +10,7 @@ #define __LINUX_UIO_H #include <linux/kernel.h> +#include <linux/fs.h> #include <uapi/linux/uio.h> struct page; @@ -111,6 +112,15 @@ static inline bool iter_is_iovec(struct iov_iter *i) } /* + * Get one of READ or WRITE out of iter->type without any other flags OR'd in + * with it. + */ +static inline int iov_iter_rw(const struct iov_iter *i) +{ + return i->type & RW_MASK; +} + +/* * Cap the iov_iter by given limit; note that the second argument is * *not* the new size - it's upper limit for such. Passing it a value * greater than the amount of data in iov_iter is fine - it'll just do
Get either READ or WRITE out of iter->type. Signed-off-by: Omar Sandoval <osandov@osandov.com> --- include/linux/uio.h | 10 ++++++++++ 1 file changed, 10 insertions(+)