Re: On gratuitous modularization

[Christian Franke <Christian.Franke@t-online.de>]

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