Re: [PATCH 0/10] ide: flags query macros

[Borislav Petkov]

Hi,

> Unfortunately my main concern is still not addressed -- namely the lack of
> consistency between names of flags and names of inline functions, ie.:
> 
> -               i, (drive->dev_flags & IDE_DFLAG_NO_IO_32BIT) ? "off" : "on");
> +               i, ide_dev_no_32bit_io(drive) ? "off" : "on");
> 
> This is really the major issue because introduction of this abstraction
> was supposed to make code more readable and maintainable...
> 
> With the current version I get exactly the opposite feeling:
> - we have now different naming used for flags and inline functions
> - we use inline functions only for checking if flags are set

Sorry about that, will think of better names and fix.

> My other complaint is about changing my beloved CodingStyle, i.e.:
> 
> -       if (drive->media != ide_disk ||
> -           (drive->dev_flags & IDE_DFLAG_PRESENT) == 0)
> +       if (drive->media != ide_disk || !ide_dev_present(drive))
>                 select |= HT_PREFETCH_MODE;
> 
> I see '== 0' immediately while it takes a while to notice '!' (funny that
> long time ago I preferred '!' because it's shorter but in the practice it
> turns out to be less readable and more prone to cause subtle bugs during
> code changes, though YMMV).

:) this is funny, I feel the exact opposite way: If I see the "!" at the
beginning of the if-clause I just read "not" together with the function
name so for example for

if (!ide_dev_present(drive))

you have "if ide device _not_ present" or even more closely matched word
order would be "if not ide device present", well, you get the idea.
That's one of the reasons I was trying to have more readable names
for those inlines. And this way it is much more natural when reading
the code instead of "== 0" check where you still have to think a bit :).

> Also after checking the code I think ide_{d,a}flag_ naming is better
> as it doesn't overlap with normal ide code...
> 
> > > diff --git a/drivers/ide/ide-cd_ioctl.c b/drivers/ide/ide-cd_ioctl.c
> > > index df3df00..3553759 100644
> > > --- a/drivers/ide/ide-cd_ioctl.c
> > > +++ b/drivers/ide/ide-cd_ioctl.c
> > > @@ -86,7 +86,7 @@ int ide_cdrom_check_media_change_real(struct cdrom_device_info *cdi,
> > > 
> > >         if (slot_nr == CDSL_CURRENT) {
> > >                 (void) cdrom_check_status(drive, NULL);
> > > -               retval = (drive->dev_flags & IDE_DFLAG_MEDIA_CHANGED) ? 1 : 0;
> > > +               retval = ide_dev_media_changed(drive) ? 1 : 0;
> > >                 drive->dev_flags &= ~IDE_DFLAG_MEDIA_CHANGED;
> > >                 return retval;
> > >         } else {
> > > 
> > > The use of ? 1 : 0; here is redundant.
> > > 
> > >                         if (drive->media == ide_disk) {
> > > -                               printk(KERN_INFO "%s: non-IDE drive, CHS=%d/%d/%d\n",
> > > +                               pr_info("%s: non-IDE drive, CHS=%d/%d/%d\n",
> > >                                         drive->name, drive->cyl,
> > >                                         drive->head, drive->sect);
> > >                         } else if (drive->media == ide_cdrom) {
> > > -                               printk(KERN_INFO "%s: ATAPI cdrom (?)\n", drive->name);
> > > +                               pr_info("%s: ATAPI cdrom (?)\n", drive->name);
> > >                         } else {
> > >                                 /* nuke it */
> > > -                               printk(KERN_WARNING "%s: Unknown device on bus refused identification. Ignoring.\n",
> > > +drive->name);
> > > +                               pr_warning("%s: Unknown device on bus refused "
> > > +                                          "identification, ignoring.\n",
> > > +                                          drive->name);
> > > I did not see this addressed in the changelog?
> > 
> > Actually, that was in the v1 changelog but got forgotten. Bart, can you
> > please add to the changelog
> > 
> > - shorten >80 lines
> 
> What about printk() -> pr_info()/pr_warning()?
> 
> [ Which brings us to consistency issues again -- do you plan to convert
>   whole IDE code to use pr_*()?  If yes, great but please do it in separate
>   patches -- I think that converting only some printk()s is not worth it. ]

Well, I don't know, this could just as well be a kernel janitor task.
I'll revert to printks here since there's more important stuff to do
now.

> Please spend more time on documenting your changes properly.
> 
> You don't have to write a poem ;) but for reviewer it is important
> to know if changes are intentional or accidental (since it could be 
> as well unintentional left-over from your private tree)...

Point taken.

> > >                                 drive->dev_flags &= ~IDE_DFLAG_PRESENT;
> > > 
> > > You have a nice set of inlines to facilitate testing bits,
> > > but not for the above use?
> > > I guess this was not worth the abstraction for now.
> > 
> > Yeah, those are next but I'd like to wait a bit until ide rewrite
> > settles...
> 
> This should happen in this patch to keep the consistency, moreover since
> you introduced nice macros to define "test" helpers you can now easily extend
> them for "clear" ones, i.e.:
> 
> +#define IDE_AFLAG_(name, flag) \
> +static inline int ide_test_aflag_##name(ide_drive_t *drive) \
> +{ \
> +       return !!(drive->atapi_flags & flag); \
> +} \
> +static inline void ide_clear_aflag_##name(ide_drive_t *drive) \
> +{ \
> +       drive->atapi_flags &= ~flag; \
> +}
> 
> BTW you may want to delay this patch after 2.6.30 as things should
> become much more peaceful then.

What exactly is the timeframe here? Do you want to have the updated
version for the 2.6.31 merge window?

-- 
Regards/Gruss,
    Boris.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/