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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
