Re: On gratuitous modularization

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- 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 --]