On Sat, 28 Apr 2007 06:23:54 -0400 (EDT) "Robert P. J. Day" <[email protected]> wrote:
> Replace the MAJOR() macro invocation with a call to the inline
> imajor() routine.
>
> Signed-off-by: Robert P. J. Day <[email protected]>
>
> ---
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 6b5b642..08da15b 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -710,7 +710,7 @@ static inline int is_loop_device(struct file *file)
> {
> struct inode *i = file->f_mapping->host;
>
> - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
> + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
> }
there's no runtime change, and I count a couple hundred MAJORs in the tree.
I don't want to receive 200 one-line patches please. If you're going to
do this then please do decent-sized per-subsystem patches and see if you can
persuade the subsystem maintainers to take them directly.
On Thu, 3 May 2007, Andrew Morton wrote:
> On Sat, 28 Apr 2007 06:23:54 -0400 (EDT) "Robert P. J. Day" <[email protected]> wrote:
>
> > Replace the MAJOR() macro invocation with a call to the inline
> > imajor() routine.
> >
> > Signed-off-by: Robert P. J. Day <[email protected]>
> >
> > ---
> >
> > diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> > index 6b5b642..08da15b 100644
> > --- a/drivers/block/loop.c
> > +++ b/drivers/block/loop.c
> > @@ -710,7 +710,7 @@ static inline int is_loop_device(struct file *file)
> > {
> > struct inode *i = file->f_mapping->host;
> >
> > - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
> > + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
> > }
>
> there's no runtime change, and I count a couple hundred MAJORs in
> the tree.
>
> I don't want to receive 200 one-line patches please. If you're
> going to do this then please do decent-sized per-subsystem patches
> and see if you can persuade the subsystem maintainers to take them
> directly.
you misunderstand the point of that patch. it's not to replace all
instances of MAJOR(), only those that are being used in specifically
that context -- to extract the major (or minor) number from an inode,
and there's a *very* small number of those:
$ grep -Er "(MINOR|MAJOR).*i_rdev" *
arch/sh/boards/landisk/landisk_pwb.c: minor = MINOR(inode->i_rdev);
arch/sh/boards/landisk/landisk_pwb.c: minor = MINOR(inode->i_rdev);
drivers/block/loop.c: return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
drivers/media/video/ivtv/ivtv-fileops.c: int minor = MINOR(inode->i_rdev);
include/linux/fs.h: return MINOR(inode->i_rdev);
include/linux/fs.h: return MAJOR(inode->i_rdev);
sound/oss/au1550_ac97.c: int minor = MINOR(inode->i_rdev);
it's just standardizing on using the imajor() and iminor() inlines
defined in include/linux/fs.h.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On May 3 2007 23:18, Andrew Morton wrote:
>> struct inode *i = file->f_mapping->host;
>>
>> - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
>> + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
>> }
>
>there's no runtime change, and I count a couple hundred MAJORs in the tree.
Why do we even have imajor() if all it does is calling the MAJOR()
macro?
Jan
--
On Fri, 4 May 2007, Jan Engelhardt wrote:
>
> On May 3 2007 23:18, Andrew Morton wrote:
> >> struct inode *i = file->f_mapping->host;
> >>
> >> - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
> >> + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
> >> }
> >
> >there's no runtime change, and I count a couple hundred MAJORs in the tree.
>
> Why do we even have imajor() if all it does is calling the MAJOR()
> macro?
i'm guessing it's to hide the underlying implementation of
extracting the major/minor numbers from an inode, in case that
implementation ever changes, which strikes me as perfectly reasonable.
and i don't think you'd have any luck arguing that it should be
removed at this point:
$ grep -Erw "(imajor|iminor)" * | wc -l
350
all i was doing was standardizing the small handful of holdouts.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On May 4 2007 04:14, Robert P. J. Day wrote:
>> On May 3 2007 23:18, Andrew Morton wrote:
>> >> struct inode *i = file->f_mapping->host;
>> >>
>> >> - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
>> >> + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
>> >> }
>> >
>> >there's no runtime change, and I count a couple hundred MAJORs in the tree.
>>
>> Why do we even have imajor() if all it does is calling the MAJOR()
>> macro?
>
> i'm guessing it's to hide the underlying implementation of
>extracting the major/minor numbers from an inode, in case that
>implementation ever changes, which strikes me as perfectly reasonable.
How often has the implementation changed? I think i_rdev has been
there for a looong time. But yes, doing the MAJOR => imajor conversion
is preferable. Because you don't need the struct declaration for inode
then, and may omit to #include <linux/fs.h>. (Other things may need
fs.h so it's a bit of a corner case.)
> all i was doing was standardizing the small handful of holdouts.
Please continue, this was not a rant :)
Jan
--
On Fri, 4 May 2007, Jan Engelhardt wrote:
> On May 4 2007 04:14, Robert P. J. Day wrote:
> >> On May 3 2007 23:18, Andrew Morton wrote:
> >> >> struct inode *i = file->f_mapping->host;
> >> >>
> >> >> - return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
> >> >> + return i && S_ISBLK(i->i_mode) && imajor(i) == LOOP_MAJOR;
> >> >> }
> >> >
> >> >there's no runtime change, and I count a couple hundred MAJORs in the tree.
> >>
> >> Why do we even have imajor() if all it does is calling the MAJOR()
> >> macro?
> >
> > i'm guessing it's to hide the underlying implementation of
> >extracting the major/minor numbers from an inode, in case that
> >implementation ever changes, which strikes me as perfectly reasonable.
>
> How often has the implementation changed? I think i_rdev has been
> there for a looong time. But yes, doing the MAJOR => imajor
> conversion is preferable. Because you don't need the struct
> declaration for inode then, and may omit to #include <linux/fs.h>.
> (Other things may need fs.h so it's a bit of a corner case.)
>
> > all i was doing was standardizing the small handful of holdouts.
>
> Please continue, this was not a rant :)
that was it, that one-line patch. as i pointed out, the entire source
tree currently contains precisely this set of holdouts:
arch/sh/boards/landisk/landisk_pwb.c: minor = MINOR(inode->i_rdev);
arch/sh/boards/landisk/landisk_pwb.c: minor = MINOR(inode->i_rdev);
drivers/block/loop.c: return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
drivers/media/video/ivtv/ivtv-fileops.c: int minor = MINOR(inode->i_rdev);
include/linux/fs.h: return MINOR(inode->i_rdev);
include/linux/fs.h: return MAJOR(inode->i_rdev);
sound/oss/au1550_ac97.c: int minor = MINOR(inode->i_rdev);
the first two are SH-related and i mailed the SH maintainer offline
about them, that previous patch was for the loop.c file, i'm not sure
who's in charge of video/ivtv (hints?), the next two lines are the
header files that define the functions, and i long ago decided not to
invest any time cleaning anything in the OSS directory.
so, really, it's all of four lines that need to be updated to
complete the job.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry
Waterloo, Ontario, CANADA
http://fsdev.net/wiki/index.php?title=Main_Page
========================================================================
On May 4 2007 08:21, Robert P. J. Day wrote:
>arch/sh/boards/landisk/landisk_pwb.c: minor = MINOR(inode->i_rdev);
>arch/sh/boards/landisk/landisk_pwb.c: minor = MINOR(inode->i_rdev);
>drivers/block/loop.c: return i && S_ISBLK(i->i_mode) && MAJOR(i->i_rdev) == LOOP_MAJOR;
>drivers/media/video/ivtv/ivtv-fileops.c: int minor = MINOR(inode->i_rdev);
>include/linux/fs.h: return MINOR(inode->i_rdev);
>include/linux/fs.h: return MAJOR(inode->i_rdev);
>sound/oss/au1550_ac97.c: int minor = MINOR(inode->i_rdev);
>
>who's in charge of video/ivtv (hints?)
>From maintainers:
VIDEO FOR LINUX
P: Mauro Carvalho Chehab
M: [email protected]
M: [email protected]
L: [email protected]
W: http://linuxtv.org
T: git kernel.org:/pub/scm/linux/kernel/git/mchehab/v4l-dvb.git
S: Maintained
Jan
--