From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1KaeGn-00085C-0f for mharc-grub-devel@gnu.org; Tue, 02 Sep 2008 18:22:41 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KaeGk-00084p-O7 for grub-devel@gnu.org; Tue, 02 Sep 2008 18:22:38 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KaeGj-00084c-Ae for grub-devel@gnu.org; Tue, 02 Sep 2008 18:22:37 -0400 Received: from [199.232.76.173] (port=57607 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KaeGj-00084Z-5G for grub-devel@gnu.org; Tue, 02 Sep 2008 18:22:37 -0400 Received: from fg-out-1718.google.com ([72.14.220.157]:38609) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KaeGi-0003BW-Td for grub-devel@gnu.org; Tue, 02 Sep 2008 18:22:37 -0400 Received: by fg-out-1718.google.com with SMTP id l26so1695701fgb.30 for ; Tue, 02 Sep 2008 15:22:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from :user-agent:mime-version:to:subject:references:in-reply-to :x-enigmail-version:content-type; bh=sRIiZ3DkJHVRZNyaal2jHh4TJvZqoh34zzZTppKrOyk=; b=MkxHpakb4ZDOTiE8GbEyIdOF8fAL4FHlSVgoMOKh7k4C5y0mj1/aHwpmj8xMh8kBtR UH/c71wkNVQsPE5h6ASJNoW1A1tHhUs26BoPnpicFfHAB/r4y/5mWxmRUPoVLZwPHd6d QI5b0PbwvwhnYrfmU4TgLmd9DjlV7CMVdIZzw= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:user-agent:mime-version:to:subject:references :in-reply-to:x-enigmail-version:content-type; b=sqwnh28FSeVmIUwg94Ulj6Rwmf8RAlggqYC8gV5pfw01cCjYjUYN82aPX24b9iK0e9 sQJBcpJzdoUf1xXWmoTwIslnG6aizwwVfngqOJ59DIGbh1jrOJkXHPqn7nkpsd+uptcM rYcLJF12i99sBpoTGgA4+YQHs6xY3/fi+4Tw0= Received: by 10.86.98.10 with SMTP id v10mr6051281fgb.39.1220394148940; Tue, 02 Sep 2008 15:22:28 -0700 (PDT) Received: from ?192.168.1.15? ( [83.76.170.177]) by mx.google.com with ESMTPS id d6sm7663221fga.2.2008.09.02.15.22.15 (version=TLSv1/SSLv3 cipher=RC4-MD5); Tue, 02 Sep 2008 15:22:19 -0700 (PDT) Message-ID: <48BDBC96.3010602@gmail.com> Date: Wed, 03 Sep 2008 00:22:14 +0200 From: phcoder User-Agent: Thunderbird 2.0.0.16 (X11/20080724) MIME-Version: 1.0 To: The development of GRUB 2 References: <48BD4C52.6040308@gmail.com> <1220367299.23879.15.camel@localhost> <48BD62BE.7090507@gmail.com> <1220373059.23879.25.camel@localhost> <48BD8847.9030502@gmail.com> <1220386216.23879.55.camel@localhost> In-Reply-To: <1220386216.23879.55.camel@localhost> X-Enigmail-Version: 0.95.0 Content-Type: multipart/mixed; boundary="------------080702000704070406070609" X-detected-kernel: by monty-python.gnu.org: Linux 2.6 (newer, 2) Subject: Re: Sendkey patch X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Sep 2008 22:22:39 -0000 This is a multi-part message in MIME format. --------------080702000704070406070609 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Javier Martín wrote: > El mar, 02-09-2008 a las 20:39 +0200, phcoder escribió: >> +void >> +grub_loader_remove_preboot (void *p) >> +{ >> + if (!p) >> + return; >> + *(PREBOOT_HND(p)->prev_pointer)=PREBOOT_HND(p)->next; > This line will "crash" if p is the head of the list (with prev_pointer > being 0). I quote crash because a crash is what happens under an OS: > under GRUB you just overwrite address 0x0 which in i386-pc is the start > of the real mode IVT. Thank you for pointing at this mistake. Corrected. I should really go to bed now. > >> + if (PREBOOT_HND(p)->next) >> + PREBOOT_HND(p)->next->prev_pointer=PREBOOT_HND(p)->prev_pointer; >> + grub_free (p); >> +} > All these macro plays are nonsense and a hindrance to readability just > because you did not want to add a local variable and do the cast once. > > Here is my "version" of your patch, without the double indirection and > the strange plays. The overhead is 103 bytes without the error line > against 63 of yours, but I really think that the symmetric and > understandable handling of previous and next is worth 40 bytes. > Well let's summ up what we have: -my version in 63 bytes but difficult to read (could some comments help with it?) -your version much easier to read but in 103 bytes Neither of versions is right or wrong. It's a question of priorities. I had some experiences that past some point it's difficult to decrease size past some point. Core image has to be embed in first cylinder. There we have 62 sectors=31744 bytes. And now our core image (my version with error reporting) with ata,pc and reiserfs is 29654 bytes. This leaves us 2090 bytes. This combination is not something completely out of the ordinary. So I suggest to give the priority to the size of the kernel over readability (perhaps adding some comments to my version). > PS: >> +void *EXPORT_FUNC(grub_loader_add_preboot) (grub_err_t ... > I think that (only) in function declarations it's better to write "void* > f()" than "void *f()" because otherwise the * can be easily overlooked. > However, this is my word and does not come from the GCS. > I've just made the same that I see in include/grub/mm.h . But for me it doesn't really matter Vladimir Serbinenko --------------080702000704070406070609 Content-Type: text/x-diff; name="preboot.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="preboot.patch" Index: kern/loader.c =================================================================== --- kern/loader.c (revision 1845) +++ kern/loader.c (working copy) @@ -22,12 +22,54 @@ #include #include +#define PREBOOT_HND(x) (((struct grub_preboot_t *)x)) + +struct grub_preboot_t +{ + grub_err_t (*preboot_func) (int); + struct grub_preboot_t *next; + struct grub_preboot_t **prev_pointer; +}; + static grub_err_t (*grub_loader_boot_func) (void); static grub_err_t (*grub_loader_unload_func) (void); static int grub_loader_noreturn; static int grub_loader_loaded; +static struct grub_preboot_t *grub_loader_preboots=0; + +void * +grub_loader_add_preboot (grub_err_t (*preboot_func) (int)) +{ + struct grub_preboot_t *cur; + if (!preboot_func) + return 0; + cur=(struct grub_preboot_t *)grub_malloc (sizeof (struct grub_preboot_t)); + if (!cur) + { + grub_error (GRUB_ERR_OUT_OF_MEMORY, "hook not added"); + return 0; + } + cur->next=grub_loader_preboots; + cur->prev_pointer=&grub_loader_preboots; + cur->next->prev_pointer=&(cur->next); + cur->preboot_func=preboot_func; + grub_loader_preboots=cur; + return cur; +} + +void +grub_loader_remove_preboot (void *p) +{ + if (!p) + return; + *(PREBOOT_HND(p)->prev_pointer)=PREBOOT_HND(p)->next; + if (PREBOOT_HND(p)->next) + PREBOOT_HND(p)->next->prev_pointer=PREBOOT_HND(p)->prev_pointer; + grub_free (p); +} + int grub_loader_is_loaded (void) { @@ -64,12 +106,18 @@ grub_err_t grub_loader_boot (void) { + struct grub_preboot_t *iter=grub_loader_preboots; if (! grub_loader_loaded) return grub_error (GRUB_ERR_NO_KERNEL, "no loaded kernel"); if (grub_loader_noreturn) grub_machine_fini (); - + while (iter) + { + if (iter->preboot_func) + iter->preboot_func (grub_loader_noreturn); + iter=iter->next; + } return (grub_loader_boot_func) (); } Index: include/grub/loader.h =================================================================== --- include/grub/loader.h (revision 1845) +++ include/grub/loader.h (working copy) @@ -25,6 +25,7 @@ #include #include + /* Check if a loader is loaded. */ int EXPORT_FUNC(grub_loader_is_loaded) (void); @@ -37,6 +38,12 @@ /* Unset current loader, if any. */ void EXPORT_FUNC(grub_loader_unset) (void); +/*Add a preboot function*/ +void *EXPORT_FUNC(grub_loader_add_preboot) (grub_err_t (*preboot_func) (int noreturn)); + +/*Remove given preboot function*/ +void EXPORT_FUNC(grub_loader_remove_preboot) (void *hnd); + /* Call the boot hook in current loader. This may or may not return, depending on the setting by grub_loader_set. */ grub_err_t EXPORT_FUNC(grub_loader_boot) (void); --------------080702000704070406070609--