On gratuitous modularization

On gratuitous modularization

[Robert Millan]

Please be careful when adding modules.  I see that too often new modules
are added without any real need to host this code separately.

There are many examples where this happened.  I just noticed:

commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
commands/hdparm.c:  if (grub_disk_ata_pass_through (disk, &apt))
commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
commands/hdparm.c:  if (grub_disk_ata_pass_through (disk, &apt))
commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
commands/hdparm.c:  if (grub_disk_ata_pass_through (disk, &apt))
commands/hdparm.c:  if (! grub_disk_ata_pass_through)
disk/ata_pthru.c:                      struct grub_disk_ata_pass_through_parms *parms)
disk/ata_pthru.c:  grub_disk_ata_pass_through = grub_ata_pass_through;
disk/ata_pthru.c:  if (grub_disk_ata_pass_through == grub_ata_pass_through)
disk/ata_pthru.c:    grub_disk_ata_pass_through = NULL;
include/grub/disk.h:struct grub_disk_ata_pass_through_parms
include/grub/disk.h:extern grub_err_t (* EXPORT_VAR(grub_disk_ata_pass_through)) (grub_disk_t,
include/grub/disk.h:               struct grub_disk_ata_pass_through_parms *);
kern/disk.c:grub_err_t (* grub_disk_ata_pass_through) (grub_disk_t,
kern/disk.c:        struct grub_disk_ata_pass_through_parms *);

this seems unnecessary.  ata_pthru is very small.  If it's only used by hdparm,
why not just merge it?  This also avoids the additional code in kernel.

A similar example is aout.mod, which is just a helper for loaders of *BSD
kernels.

Please also see Okuji's article on this subject:

  http://grub.enbug.org/OnSplittingModules

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: On gratuitous modularization

[Christian Franke]

Hi Robert,

Robert Millan wrote:

> Please be careful when adding modules.  I see that too often new modules
> are added without any real need to host this code separately.
>
> There are many examples where this happened.  I just noticed:
>
> commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
> commands/hdparm.c:  if (grub_disk_ata_pass_through (disk,&apt))
> commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
> commands/hdparm.c:  if (grub_disk_ata_pass_through (disk,&apt))
> commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
> commands/hdparm.c:  if (grub_disk_ata_pass_through (disk,&apt))
> commands/hdparm.c:  if (! grub_disk_ata_pass_through)
> disk/ata_pthru.c:                      struct grub_disk_ata_pass_through_parms *parms)
> disk/ata_pthru.c:  grub_disk_ata_pass_through = grub_ata_pass_through;
> disk/ata_pthru.c:  if (grub_disk_ata_pass_through == grub_ata_pass_through)
> disk/ata_pthru.c:    grub_disk_ata_pass_through = NULL;
> include/grub/disk.h:struct grub_disk_ata_pass_through_parms
> include/grub/disk.h:extern grub_err_t (* EXPORT_VAR(grub_disk_ata_pass_through)) (grub_disk_t,
> include/grub/disk.h:               struct grub_disk_ata_pass_through_parms *);
> kern/disk.c:grub_err_t (* grub_disk_ata_pass_through) (grub_disk_t,
> kern/disk.c:        struct grub_disk_ata_pass_through_parms *);
>
> this seems unnecessary.  ata_pthru is very small.  If it's only used by hdparm,
> why not just merge it?  This also avoids the additional code in kernel.
>
>    

The module ata_pthru.mod exists only to keep ata.mod small, see:
http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00091.html

Keeping the ata.mod specific pass-through function separate from 
hdparm.mod was intentional. Merging this function into hdparm.mod would 
only make sense if ata.mod will the only ATA access module with 
pass-through functionality in the future. Hdparm.mod would then depend 
on ata.mod. A 'hdparm -h' would load ata.mod and disable biosdisk access.

I hope we will eventually have an ahci.mod :-)

BTW: I agree that using a global function pointer 
'grub_disk_ata_pass_through' is a hack. A cleaner design would be 
possible with a grub_disk_dev.ioctl(.) call.

-- 
Christian Franke

Re: On gratuitous modularization

[Robert Millan]

On Mon, Jan 25, 2010 at 09:13:11PM +0100, Christian Franke wrote:
>
> The module ata_pthru.mod exists only to keep ata.mod small, see:
> http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00091.html

I see.  Thanks for pointing this out.

> Keeping the ata.mod specific pass-through function separate from  
> hdparm.mod was intentional. Merging this function into hdparm.mod would  
> only make sense if ata.mod will the only ATA access module with  
> pass-through functionality in the future.

Note that linking ata_pthru.c into hdparm.mod doesn't prevent this code from
being linked into other modules if/when the need arises.

But right now, it's only used by hdparm.  I see little justification in
having a separate module in this situation.  I'm inclined to merge it.

> BTW: I agree that using a global function pointer  
> 'grub_disk_ata_pass_through' is a hack. A cleaner design would be  
> possible with a grub_disk_dev.ioctl(.) call.

The problem I see with 'grub_disk_ata_pass_through' is that it shouldn't be
in kernel.  The interface itself is less relevant IMO.

-- 
Robert Millan

  "Be the change you want to see in the world" -- Gandhi

Re: On gratuitous modularization

[Vladimir 'φ-coder/phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

Christian Franke wrote:
> Hi Robert,
>
> Robert Millan wrote:
>
>> Please be careful when adding modules.  I see that too often new modules
>> are added without any real need to host this code separately.
>>
>> There are many examples where this happened.  I just noticed:
>>
>> commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
>> commands/hdparm.c:  if (grub_disk_ata_pass_through (disk,&apt))
>> commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
>> commands/hdparm.c:  if (grub_disk_ata_pass_through (disk,&apt))
>> commands/hdparm.c:  struct grub_disk_ata_pass_through_parms apt;
>> commands/hdparm.c:  if (grub_disk_ata_pass_through (disk,&apt))
>> commands/hdparm.c:  if (! grub_disk_ata_pass_through)
>> disk/ata_pthru.c:                      struct
>> grub_disk_ata_pass_through_parms *parms)
>> disk/ata_pthru.c:  grub_disk_ata_pass_through = grub_ata_pass_through;
>> disk/ata_pthru.c:  if (grub_disk_ata_pass_through ==
>> grub_ata_pass_through)
>> disk/ata_pthru.c:    grub_disk_ata_pass_through = NULL;
>> include/grub/disk.h:struct grub_disk_ata_pass_through_parms
>> include/grub/disk.h:extern grub_err_t (*
>> EXPORT_VAR(grub_disk_ata_pass_through)) (grub_disk_t,
>> include/grub/disk.h:               struct
>> grub_disk_ata_pass_through_parms *);
>> kern/disk.c:grub_err_t (* grub_disk_ata_pass_through) (grub_disk_t,
>> kern/disk.c:        struct grub_disk_ata_pass_through_parms *);
>>
>> this seems unnecessary.  ata_pthru is very small.  If it's only used
>> by hdparm,
>> why not just merge it?  This also avoids the additional code in kernel.
>>
>>    
>
> The module ata_pthru.mod exists only to keep ata.mod small, see:
> http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00091.html
>
> Keeping the ata.mod specific pass-through function separate from
> hdparm.mod was intentional. Merging this function into hdparm.mod
> would only make sense if ata.mod will the only ATA access module with
> pass-through functionality in the future. Hdparm.mod would then depend
> on ata.mod. A 'hdparm -h' would load ata.mod and disable biosdisk access.
>
> I hope we will eventually have an ahci.mod :-)
>
Are the parameters of current ata_pass_through ATA-specific or would
they be the same on AHCI?
> BTW: I agree that using a global function pointer
> 'grub_disk_ata_pass_through' is a hack. A cleaner design would be
> possible with a grub_disk_dev.ioctl(.) call.
>


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 293 bytes --]

Re: On gratuitous modularization

[Christian Franke]

Vladimir 'φ-coder/phcoder' Serbinenko wrote:
> Christian Franke wrote:
>    
>> Hi Robert,
>>
>> Robert Millan wrote:
>>
>>      
>>> Please be careful when adding modules.  I see that too often new modules
>>> are added without any real need to host this code separately.
>>>
>>> ...
>>> kern/disk.c:        struct grub_disk_ata_pass_through_parms *);
>>>
>>> this seems unnecessary.  ata_pthru is very small.  If it's only used
>>> by hdparm,
>>> why not just merge it?  This also avoids the additional code in kernel.
>>>
>>>
>>>        
>> The module ata_pthru.mod exists only to keep ata.mod small, see:
>> http://lists.gnu.org/archive/html/grub-devel/2009-02/msg00091.html
>>
>> Keeping the ata.mod specific pass-through function separate from
>> hdparm.mod was intentional. Merging this function into hdparm.mod
>> would only make sense if ata.mod will the only ATA access module with
>> pass-through functionality in the future. Hdparm.mod would then depend
>> on ata.mod. A 'hdparm -h' would load ata.mod and disable biosdisk access.
>>
>> I hope we will eventually have an ahci.mod :-)
>>
>>      
> Are the parameters of current ata_pass_through ATA-specific or would
> they be the same on AHCI?
>    

Like e.g. the Linux HDIO_DRIVE_TASKFILE ioctl, the ata_pass_through() 
parameters are not controller or transport specific. It should work with 
any possible future implementation of ATA pass-through functionality, e.g.
- native AHCI driver
- other native drivers
- SAT (ANSI INCITS 431-2007) ATA pass through command for:
-- SATA drives behind USB-bridges with SAT support, or:
-- SATA drives in SAS enclosures

Merging ata_pthru.mod into hdparm.mod would make hdparm specific to one 
(outdated) class of controllers: the traditional IDE-controller (T13/1510D).


>> BTW: I agree that using a global function pointer
>> 'grub_disk_ata_pass_through' is a hack. A cleaner design would be
>> possible with a grub_disk_dev.ioctl(.) call.
>>
>>      

Such an ioctl() call would IMO be the best way to handle such device 
class specific extra functionality.

-- 
Christian Franke