[PATCH 1/3] Don't initialize module before grub_dl_add() succeeds

[PATCH 1/3] Don't initialize module before grub_dl_add() succeeds

[Pavel Roskin]

ChangeLog:

	* kern/dl.c (grub_dl_load_core): Call grub_dl_call_init() only
	after grub_dl_add() succeeds.  Set mod->ref_count to 1 later to
	allow grub_dl_unload() to work.
	Original patch by Joe Auricchio <jauricchio@gmail.com>
---
 kern/dl.c |   14 +++++---------
 1 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/kern/dl.c b/kern/dl.c
index 78ebc1e..ebde547 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -539,14 +539,13 @@ grub_dl_load_core (void *addr, grub_size_t size)
   if (! mod)
     return 0;
 
-  mod->ref_count = 1;
-
   grub_dprintf ("modules", "relocating to %p\n", mod);
   if (grub_dl_resolve_name (mod, e)
       || grub_dl_resolve_dependencies (mod, e)
       || grub_dl_load_segments (mod, e)
       || grub_dl_resolve_symbols (mod, e)
-      || grub_arch_dl_relocate_symbols (mod, e))
+      || grub_arch_dl_relocate_symbols (mod, e)
+      || grub_dl_add (mod))
     {
       mod->fini = 0;
       grub_dl_unload (mod);
@@ -557,13 +556,10 @@ grub_dl_load_core (void *addr, grub_size_t size)
 
   grub_dprintf ("modules", "module name: %s\n", mod->name);
   grub_dprintf ("modules", "init function: %p\n", mod->init);
-  grub_dl_call_init (mod);
 
-  if (grub_dl_add (mod))
-    {
-      grub_dl_unload (mod);
-      return 0;
-    }
+  mod->ref_count = 1;
+
+  grub_dl_call_init (mod);
 
   return mod;
 }

[PATCH 2/3] Eliminate grub_dl_call_init()

[Pavel Roskin]

It's just two lines long and there is only one caller.  Besides, there
is no equivalent for mod->fini.

ChangeLog:

	* kern/dl.c (grub_dl_call_init): Remove.
	(grub_dl_load_core): Call mod->init directly.
---
 kern/dl.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/kern/dl.c b/kern/dl.c
index ebde547..e2382d6 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -393,13 +393,6 @@ grub_dl_resolve_symbols (grub_dl_t mod, Elf_Ehdr *e)
   return GRUB_ERR_NONE;
 }
 
-static void
-grub_dl_call_init (grub_dl_t mod)
-{
-  if (mod->init)
-    (mod->init) (mod);
-}
-
 static grub_err_t
 grub_dl_resolve_name (grub_dl_t mod, Elf_Ehdr *e)
 {
@@ -559,7 +552,8 @@ grub_dl_load_core (void *addr, grub_size_t size)
 
   mod->ref_count = 1;
 
-  grub_dl_call_init (mod);
+  if (mod->init)
+    (mod->init) (mod);
 
   return mod;
 }

[PATCH 3/3] Merge struct grub_dl_list into struct grub_dl

[Pavel Roskin]

There is no need to keep a separate list of the modules.  It requires
additional memory allocation and additional error handling.  Now
grub_dl_add() can only fail if the module is already loaded.

ChangeLog:

	* include/grub/dl.h (struct grub_dl): Add next field.
	* kern/dl.c: Remove struct grub_dl_list.  Use mod->next to
	iterate over modules.
---
 include/grub/dl.h |    1 +
 kern/dl.c         |   55 +++++++++++++++++++----------------------------------
 2 files changed, 21 insertions(+), 35 deletions(-)

diff --git a/include/grub/dl.h b/include/grub/dl.h
index 3f8b328..55cc6cc 100644
--- a/include/grub/dl.h
+++ b/include/grub/dl.h
@@ -82,6 +82,7 @@ struct grub_dl
   Elf_Sym *symtab;
   void (*init) (struct grub_dl *mod);
   void (*fini) (void);
+  struct grub_dl *next;
 };
 typedef struct grub_dl *grub_dl_t;
 
diff --git a/kern/dl.c b/kern/dl.c
index e2382d6..81c211e 100644
--- a/kern/dl.c
+++ b/kern/dl.c
@@ -40,31 +40,17 @@
 
 \f
 
-struct grub_dl_list
-{
-  struct grub_dl_list *next;
-  grub_dl_t mod;
-};
-typedef struct grub_dl_list *grub_dl_list_t;
-
-static grub_dl_list_t grub_dl_head;
+static grub_dl_t grub_dl_head;
 
 static grub_err_t
 grub_dl_add (grub_dl_t mod)
 {
-  grub_dl_list_t l;
-
   if (grub_dl_get (mod->name))
     return grub_error (GRUB_ERR_BAD_MODULE,
 		       "`%s' is already loaded", mod->name);
 
-  l = (grub_dl_list_t) grub_malloc (sizeof (*l));
-  if (! l)
-    return grub_errno;
-
-  l->mod = mod;
-  l->next = grub_dl_head;
-  grub_dl_head = l;
+  mod->next = grub_dl_head;
+  grub_dl_head = mod;
 
   return GRUB_ERR_NONE;
 }
@@ -72,13 +58,12 @@ grub_dl_add (grub_dl_t mod)
 static void
 grub_dl_remove (grub_dl_t mod)
 {
-  grub_dl_list_t *p, q;
+  grub_dl_t *p, q;
 
   for (p = &grub_dl_head, q = *p; q; p = &q->next, q = *p)
-    if (q->mod == mod)
+    if (q == mod)
       {
 	*p = q->next;
-	grub_free (q);
 	return;
       }
 }
@@ -86,11 +71,11 @@ grub_dl_remove (grub_dl_t mod)
 grub_dl_t
 grub_dl_get (const char *name)
 {
-  grub_dl_list_t l;
+  grub_dl_t mod;
 
-  for (l = grub_dl_head; l; l = l->next)
-    if (grub_strcmp (name, l->mod->name) == 0)
-      return l->mod;
+  for (mod = grub_dl_head; mod; mod = mod->next)
+    if (grub_strcmp (name, mod->name) == 0)
+      return mod;
 
   return 0;
 }
@@ -98,10 +83,10 @@ grub_dl_get (const char *name)
 void
 grub_dl_iterate (int (*hook) (grub_dl_t mod))
 {
-  grub_dl_list_t l;
+  grub_dl_t mod;
 
-  for (l = grub_dl_head; l; l = l->next)
-    if (hook (l->mod))
+  for (mod = grub_dl_head; mod; mod = mod->next)
+    if (hook (mod))
       break;
 }
 
@@ -684,17 +669,17 @@ grub_dl_unload_unneeded (void)
 {
   /* Because grub_dl_remove modifies the list of modules, this
      implementation is tricky.  */
-  grub_dl_list_t p = grub_dl_head;
+  grub_dl_t mod = grub_dl_head;
 
-  while (p)
+  while (mod)
     {
-      if (grub_dl_unload (p->mod))
+      if (grub_dl_unload (mod))
 	{
-	  p = grub_dl_head;
+	  mod = grub_dl_head;
 	  continue;
 	}
 
-      p = p->next;
+      mod = mod->next;
     }
 }
 
@@ -704,13 +689,13 @@ grub_dl_unload_all (void)
 {
   while (grub_dl_head)
     {
-      grub_dl_list_t p;
+      grub_dl_t mod;
 
       grub_dl_unload_unneeded ();
 
       /* Force to decrement the ref count. This will purge pre-loaded
 	 modules and manually inserted modules.  */
-      for (p = grub_dl_head; p; p = p->next)
-	p->mod->ref_count--;
+      for (mod = grub_dl_head; mod; mod = mod->next)
+	mod->ref_count--;
     }
 }

Re: [PATCH 2/3] Eliminate grub_dl_call_init()

[Vladimir 'phcoder' Serbinenko]

On Wed, Jul 22, 2009 at 2:47 AM, Pavel Roskin<proski@gnu.org> wrote:
> It's just two lines long and there is only one caller.  Besides, there
> is no equivalent for mod->fini.
This patch is fine for me
>
> ChangeLog:
>
>        * kern/dl.c (grub_dl_call_init): Remove.
>        (grub_dl_load_core): Call mod->init directly.
> ---
>  kern/dl.c |   10 ++--------
>  1 files changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/kern/dl.c b/kern/dl.c
> index ebde547..e2382d6 100644
> --- a/kern/dl.c
> +++ b/kern/dl.c
> @@ -393,13 +393,6 @@ grub_dl_resolve_symbols (grub_dl_t mod, Elf_Ehdr *e)
>   return GRUB_ERR_NONE;
>  }
>
> -static void
> -grub_dl_call_init (grub_dl_t mod)
> -{
> -  if (mod->init)
> -    (mod->init) (mod);
> -}
> -
>  static grub_err_t
>  grub_dl_resolve_name (grub_dl_t mod, Elf_Ehdr *e)
>  {
> @@ -559,7 +552,8 @@ grub_dl_load_core (void *addr, grub_size_t size)
>
>   mod->ref_count = 1;
>
> -  grub_dl_call_init (mod);
> +  if (mod->init)
> +    (mod->init) (mod);
>
>   return mod;
>  }
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

Re: [PATCH 1/3] Don't initialize module before grub_dl_add() succeeds

[Vladimir 'phcoder' Serbinenko]

On Wed, Jul 22, 2009 at 2:47 AM, Pavel Roskin<proski@gnu.org> wrote:
> ChangeLog:
>
>        * kern/dl.c (grub_dl_load_core): Call grub_dl_call_init() only
>        after grub_dl_add() succeeds.  Set mod->ref_count to 1 later to
>        allow grub_dl_unload() to work.
>        Original patch by Joe Auricchio <jauricchio@gmail.com>
Ok with me
> ---
>  kern/dl.c |   14 +++++---------
>  1 files changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/kern/dl.c b/kern/dl.c
> index 78ebc1e..ebde547 100644
> --- a/kern/dl.c
> +++ b/kern/dl.c
> @@ -539,14 +539,13 @@ grub_dl_load_core (void *addr, grub_size_t size)
>   if (! mod)
>     return 0;
>
> -  mod->ref_count = 1;
> -
>   grub_dprintf ("modules", "relocating to %p\n", mod);
>   if (grub_dl_resolve_name (mod, e)
>       || grub_dl_resolve_dependencies (mod, e)
>       || grub_dl_load_segments (mod, e)
>       || grub_dl_resolve_symbols (mod, e)
> -      || grub_arch_dl_relocate_symbols (mod, e))
> +      || grub_arch_dl_relocate_symbols (mod, e)
> +      || grub_dl_add (mod))
>     {
>       mod->fini = 0;
>       grub_dl_unload (mod);
> @@ -557,13 +556,10 @@ grub_dl_load_core (void *addr, grub_size_t size)
>
>   grub_dprintf ("modules", "module name: %s\n", mod->name);
>   grub_dprintf ("modules", "init function: %p\n", mod->init);
> -  grub_dl_call_init (mod);
>
> -  if (grub_dl_add (mod))
> -    {
> -      grub_dl_unload (mod);
> -      return 0;
> -    }
> +  mod->ref_count = 1;
> +
> +  grub_dl_call_init (mod);
>
>   return mod;
>  }
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git