Re: Sendkey patch

[phcoder <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2238 bytes --]


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


[-- Attachment #2: preboot.patch --]
[-- Type: text/x-diff, Size: 2750 bytes --]

Index: kern/loader.c
===================================================================
--- kern/loader.c	(revision 1845)
+++ kern/loader.c	(working copy)
@@ -22,12 +22,54 @@
 #include <grub/err.h>
 #include <grub/kernel.h>
 
+#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 <grub/err.h>
 #include <grub/types.h>
 
+
 /* 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);