From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1NZWrQ-0003pt-SE for mharc-grub-devel@gnu.org; Mon, 25 Jan 2010 16:52:40 -0500 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NZWrN-0003k0-FZ for grub-devel@gnu.org; Mon, 25 Jan 2010 16:52:37 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NZWrJ-0003eZ-OL for grub-devel@gnu.org; Mon, 25 Jan 2010 16:52:37 -0500 Received: from [199.232.76.173] (port=55495 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NZWrJ-0003eR-Ja for grub-devel@gnu.org; Mon, 25 Jan 2010 16:52:33 -0500 Received: from mailout02.t-online.de ([194.25.134.17]:42563) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NZWrJ-00026t-4g for grub-devel@gnu.org; Mon, 25 Jan 2010 16:52:33 -0500 Received: from fwd01.aul.t-online.de (fwd01.aul.t-online.de ) by mailout02.t-online.de with smtp id 1NZVJR-0007Dz-7r; Mon, 25 Jan 2010 21:13:29 +0100 Received: from [10.3.2.2] (X7kEbcZprh4ehhfjgkb6nRlKAD6uUrUnbj18A8HZmHyuPhNOEcToagcmF81GdS7wgm@[217.235.220.243]) by fwd01.aul.t-online.de with esmtp id 1NZVJB-0AHgPo0; Mon, 25 Jan 2010 21:13:13 +0100 Message-ID: <4B5DFB57.5020703@t-online.de> Date: Mon, 25 Jan 2010 21:13:11 +0100 From: Christian Franke User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.7) Gecko/20100104 SeaMonkey/2.0.2 MIME-Version: 1.0 To: grub-devel@gnu.org References: <20100125080620.GA14131@thorin> In-Reply-To: <20100125080620.GA14131@thorin> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-ID: X7kEbcZprh4ehhfjgkb6nRlKAD6uUrUnbj18A8HZmHyuPhNOEcToagcmF81GdS7wgm X-TOI-MSGID: 69533eea-a76d-4d68-81a0-6ea9440ed21b X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 3) Subject: Re: On gratuitous modularization X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 25 Jan 2010 21:52:37 -0000 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