Handlers

Handlers

[Marco Gerards]

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

Hi,

This patch adds handler support to GRUB 2.  This makes it easier to
add new handlers to GRUB without duplicating code over and over.  Bean
wanted this for some of his ideas :-)

Okuji (and others), can you please review this patch?  It is an
intrusive (but yet small!) change of which I hope most people agree
with.  I just don't want to commit this without proper review.

Thanks,
Marco 


2008-08-29  Marco Gerards  <marco@gnu.org>

	* conf/i386-efi.rmk (grub_emu_SOURCES): Add `kern/handler.c'.
	(kernel_img_SOURCES): Likewise.
	(grub_setup_SOURCES): Likewise.
	(kernel_mod_HEADERS): Add handler.h.

	* conf/common.rmk (grub_probe_SOURCES): Add `kern/handler.c'.
	(grub_fstest_SOURCES): Likewise.

	* conf/i386-efi.rmk (grub_emu_SOURCES): Add `kern/handler.c'.
	(kernel_mod_SOURCES): Likewise.
	(kernel_mod_HEADERS): Add `handler.h'.

	* conf/x86_64-efi.rmk (grub_emu_SOURCES): Add `kern/handler.c'.
	(kernel_mod_SOURCES): Likewise.
	(kernel_mod_HEADERS): Add `handler.h'.

	* conf/powerpc-ieee1275.rmk (kernel_elf_HEADERS): Add `handler.h'.
	(grub_emu_SOURCES): Add `kern/handler.c'.
	(kernel_elf_SOURCES): Likewise.

	* conf/i386-coreboot.rmk (kernel_elf_HEADERS): Add `handler.h'.
	(grub_emu_SOURCES): Add kern/handler.c
	(kernel_elf_SOURCES): Likewise.

	* conf/sparc64-ieee1275.rmk (kernel_elf_HEADERS): Add `handler.h'.
	(kernel_elf_SOURCES): Likewise.

	* conf/i386-ieee1275.rmk (kernel_elf_HEADERS): Add `handler.h'.
	(grub_emu_SOURCES): Add `kern/handler.c'.
	(kernel_elf_SOURCES): Likewise.

	* kern/fs.c: Register fs handler using GRUB_HANDLER_CREATE_C.
	(grub_fs_list): Removed.
	(grub_fs_register): Likewise.
	(grub_fs_unregister): Likewise.
	(grub_fs_iterate): Likewise.

	* kern/handler.c: New file.

	* include/grub/handler.h: Likewise.

	* include/grub/fs.h: Include <grub/handler.h>.  Create handler
	using GRUB_HANDLER_CREATE_H_KERNEL.
	(grub_fs): Removed members `name' and `next'.  Use
	GRUB_HANDLER_CREATE_STRUCT to recreate these members.
	(grub_fs_t): Removed.

	* genkernsyms.sh.in: Register register, unregister and iterate
	kernel symbols for each handler.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: grub2_handler.diff --]
[-- Type: text/x-diff, Size: 21028 bytes --]

Index: conf/common.rmk
===================================================================
--- conf/common.rmk	(revision 1833)
+++ conf/common.rmk	(working copy)
@@ -20,7 +20,7 @@ grub_probe_SOURCES = util/grub-probe.c	\
 	fs/ufs.c fs/xfs.c fs/afs.c				\
 	\
 	partmap/pc.c partmap/apple.c partmap/gpt.c 		\
-	kern/fs.c kern/env.c fs/fshelp.c			\
+	kern/fs.c kern/env.c kern/handler.c fs/fshelp.c		\
 	disk/lvm.c disk/raid.c disk/mdraid_linux.c grub_probe_init.c
 
 ifeq ($(enable_grub_fstest), yes)
@@ -40,8 +40,8 @@ grub_fstest_SOURCES = util/grub-fstest.c
 	fs/ufs.c fs/xfs.c fs/afs.c					\
 	\
 	kern/partition.c partmap/pc.c partmap/apple.c partmap/gpt.c 	\
-	kern/fs.c kern/env.c fs/fshelp.c disk/lvm.c disk/raid.c		\
-	disk/raid5_recover.c disk/raid6_recover.c 			\
+	kern/fs.c kern/env.c kern/handler.c fs/fshelp.c disk/lvm.c	\
+	disk/raid.c disk/raid5_recover.c disk/raid6_recover.c		\
 	disk/mdraid_linux.c disk/dmraid_nvidia.c 			\
 	grub_fstest_init.c
 
Index: conf/i386-pc.rmk
===================================================================
--- conf/i386-pc.rmk	(revision 1833)
+++ conf/i386-pc.rmk	(working copy)
@@ -40,7 +40,7 @@ cdboot_img_LDFLAGS = $(COMMON_LDFLAGS) $
 
 # For kernel.img.
 kernel_img_SOURCES = kern/i386/pc/startup.S kern/main.c kern/device.c \
-	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
+	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c kern/handler.c \
 	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
 	kern/time.c \
 	kern/i386/dl.c kern/i386/pc/init.c kern/i386/pc/mmap.c \
@@ -52,7 +52,7 @@ kernel_img_SOURCES = kern/i386/pc/startu
 	term/i386/pc/console.c \
 	symlist.c
 kernel_img_HEADERS = arg.h boot.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
+	env.h err.h file.h fs.h handler.h kernel.h loader.h misc.h mm.h net.h parser.h \
 	partition.h pc_partition.h rescue.h symbol.h term.h time.h types.h \
 	machine/biosdisk.h machine/boot.h machine/console.h machine/init.h \
 	machine/memory.h machine/loader.h machine/vga.h machine/vbe.h \
@@ -93,8 +93,9 @@ util/i386/pc/grub-mkimage.c_DEPENDENCIES
 util/i386/pc/grub-setup.c_DEPENDENCIES = grub_setup_init.h
 grub_setup_SOURCES = util/i386/pc/grub-setup.c util/biosdisk.c	\
 	util/misc.c util/getroot.c kern/device.c kern/disk.c	\
-	kern/err.c kern/misc.c kern/parser.c kern/partition.c	\
-	kern/file.c kern/fs.c kern/env.c fs/fshelp.c		\
+	kern/err.c kern/handler.c kern/misc.c kern/parser.c	\
+	kern/partition.c kern/file.c kern/fs.c kern/env.c	\
+	fs/fshelp.c						\
 	\
 	fs/affs.c fs/cpio.c fs/ext2.c fs/fat.c fs/hfs.c		\
 	fs/hfsplus.c fs/iso9660.c fs/udf.c fs/jfs.c fs/minix.c	\
@@ -124,7 +125,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	\
 	io/gzio.c							\
 	kern/device.c kern/disk.c kern/dl.c kern/elf.c kern/env.c	\
-	kern/err.c							\
+	kern/err.c kern/handler.c					\
 	normal/execute.c kern/file.c kern/fs.c normal/lexer.c 		\
 	kern/loader.c kern/main.c kern/misc.c kern/parser.c		\
 	grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c	\
Index: conf/i386-efi.rmk
===================================================================
--- conf/i386-efi.rmk	(revision 1833)
+++ conf/i386-efi.rmk	(working copy)
@@ -48,7 +48,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	\
 	io/gzio.c							\
 	kern/device.c kern/disk.c kern/dl.c kern/elf.c kern/env.c	\
-	kern/err.c							\
+	kern/err.c kern/handler.c					\
 	normal/execute.c kern/file.c kern/fs.c normal/lexer.c 		\
 	kern/loader.c kern/main.c kern/misc.c kern/parser.c		\
 	grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c	\
@@ -82,7 +82,7 @@ pkglib_MODULES = kernel.mod normal.mod _
 # For kernel.mod.
 kernel_mod_EXPORTS = no
 kernel_mod_SOURCES = kern/i386/efi/startup.S kern/main.c kern/device.c \
-	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
+	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c kern/handler.c \
 	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
 	kern/i386/dl.c kern/i386/efi/init.c kern/parser.c kern/partition.c \
 	kern/env.c symlist.c kern/efi/efi.c kern/efi/init.c kern/efi/mm.c \
@@ -92,7 +92,7 @@ kernel_mod_SOURCES = kern/i386/efi/start
 	kern/generic/rtc_get_time_ms.c \
 	kern/generic/millisleep.c
 kernel_mod_HEADERS = arg.h boot.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
+	env.h err.h handler.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
 	partition.h pc_partition.h rescue.h symbol.h term.h time.h types.h \
 	efi/efi.h efi/time.h efi/disk.h
 kernel_mod_CFLAGS = $(COMMON_CFLAGS)
Index: conf/x86_64-efi.rmk
===================================================================
--- conf/x86_64-efi.rmk	(revision 1833)
+++ conf/x86_64-efi.rmk	(working copy)
@@ -50,7 +50,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	\
 	io/gzio.c							\
 	kern/device.c kern/disk.c kern/dl.c kern/elf.c kern/env.c	\
-	kern/err.c							\
+	kern/err.c kern/handler.c					\
 	normal/execute.c kern/file.c kern/fs.c normal/lexer.c 		\
 	kern/loader.c kern/main.c kern/misc.c kern/parser.c		\
 	grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c	\
@@ -85,7 +85,7 @@ pkglib_MODULES = kernel.mod normal.mod _
 kernel_mod_EXPORTS = no
 kernel_mod_SOURCES = kern/x86_64/efi/startup.S kern/x86_64/efi/callwrap.S \
 	kern/main.c kern/device.c \
-	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
+	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c kern/handler.c \
 	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
 	kern/x86_64/dl.c kern/i386/efi/init.c kern/parser.c kern/partition.c \
 	kern/env.c symlist.c kern/efi/efi.c kern/efi/init.c kern/efi/mm.c \
@@ -94,7 +94,7 @@ kernel_mod_SOURCES = kern/x86_64/efi/sta
 	kern/generic/millisleep.c kern/generic/rtc_get_time_ms.c \
 	term/efi/console.c disk/efi/efidisk.c
 kernel_mod_HEADERS = arg.h boot.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
+	env.h err.h handler.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
 	partition.h pc_partition.h rescue.h symbol.h term.h time.h types.h \
 	efi/efi.h efi/time.h efi/disk.h machine/loader.h
 kernel_mod_CFLAGS = $(COMMON_CFLAGS)
Index: conf/i386-coreboot.rmk
===================================================================
--- conf/i386-coreboot.rmk	(revision 1833)
+++ conf/i386-coreboot.rmk	(working copy)
@@ -15,7 +15,7 @@ kernel_elf_SOURCES = kern/i386/coreboot/
 	kern/i386/coreboot/init.c \
 	kern/i386/coreboot/mmap.c \
 	kern/main.c kern/device.c \
-	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
+	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c kern/handler.c \
 	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
 	kern/time.c \
 	kern/i386/dl.c kern/parser.c kern/partition.c \
@@ -27,7 +27,7 @@ kernel_elf_SOURCES = kern/i386/coreboot/
 	term/i386/pc/at_keyboard.c term/i386/pc/vga_text.c \
 	symlist.c
 kernel_elf_HEADERS = arg.h boot.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
+	env.h err.h file.h fs.h handler.h kernel.h loader.h misc.h mm.h net.h parser.h \
 	partition.h pc_partition.h rescue.h symbol.h term.h time.h types.h \
 	machine/boot.h machine/console.h machine/init.h \
 	machine/memory.h machine/loader.h
@@ -71,7 +71,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	fs/fshelp.c							\
 	io/gzio.c							\
 	kern/device.c kern/disk.c kern/dl.c kern/elf.c kern/env.c	\
-	kern/err.c							\
+	kern/err.c kern/handler.c					\
 	normal/execute.c kern/file.c kern/fs.c normal/lexer.c 		\
 	kern/loader.c kern/main.c kern/misc.c kern/parser.c		\
 	grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c	\
Index: conf/powerpc-ieee1275.rmk
===================================================================
--- conf/powerpc-ieee1275.rmk	(revision 1833)
+++ conf/powerpc-ieee1275.rmk	(working copy)
@@ -14,7 +14,7 @@ MOSTLYCLEANFILES += symlist.c kernel_sym
 DEFSYMFILES += kernel_syms.lst
 
 kernel_elf_HEADERS = arg.h boot.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h misc.h mm.h net.h parser.h rescue.h \
+	env.h err.h file.h fs.h handler.h kernel.h misc.h mm.h net.h parser.h rescue.h \
 	symbol.h term.h time.h types.h powerpc/libgcc.h loader.h partition.h \
 	pc_partition.h ieee1275/ieee1275.h machine/kernel.h
 
@@ -53,7 +53,8 @@ grub_emu_SOURCES = commands/boot.c comma
 	\
 	io/gzio.c							\
 	kern/device.c kern/disk.c kern/dl.c kern/elf.c kern/env.c	\
-	kern/err.c kern/file.c kern/fs.c kern/loader.c kern/main.c	\
+	kern/err.c kern/file.c kern/fs.c kern/handler.c kern/loader.c	\
+	kern/main.c							\
 	kern/misc.c kern/parser.c kern/partition.c kern/rescue.c	\
 	kern/term.c fs/fshelp.c						\
 	normal/arg.c normal/cmdline.c normal/command.c			\
@@ -75,10 +76,10 @@ grub_emu_LDFLAGS = $(LIBCURSES)
 
 kernel_elf_SOURCES = kern/powerpc/ieee1275/crt0.S kern/ieee1275/cmain.c \
 	kern/ieee1275/ieee1275.c kern/main.c kern/device.c 		\
-	kern/disk.c kern/dl.c kern/err.c kern/file.c kern/fs.c 		\
-	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c 	\
-	kern/ieee1275/init.c term/ieee1275/ofconsole.c 		\
-	kern/ieee1275/openfw.c disk/ieee1275/ofdisk.c 		\
+	kern/disk.c kern/dl.c kern/err.c kern/handler.c kern/file.c	\
+	kern/fs.c kern/misc.c kern/mm.c kern/loader.c kern/rescue.c	\
+	kern/term.c kern/ieee1275/init.c term/ieee1275/ofconsole.c	\
+	kern/ieee1275/openfw.c disk/ieee1275/ofdisk.c 			\
 	kern/parser.c kern/partition.c kern/env.c kern/powerpc/dl.c 	\
 	kern/generic/millisleep.c kern/time.c                            \
 	symlist.c kern/powerpc/cache.S
Index: conf/sparc64-ieee1275.rmk
===================================================================
--- conf/sparc64-ieee1275.rmk	(revision 1833)
+++ conf/sparc64-ieee1275.rmk	(working copy)
@@ -14,7 +14,7 @@ MOSTLYCLEANFILES += symlist.c kernel_sym
 DEFSYMFILES += kernel_syms.lst
 
 kernel_elf_HEADERS = arg.h boot.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h misc.h mm.h net.h parser.h rescue.h \
+	env.h err.h handler.h file.h fs.h kernel.h misc.h mm.h net.h parser.h rescue.h \
 	symbol.h term.h time.h types.h sparc64/libgcc.h loader.h partition.h \
 	pc_partition.h ieee1275/ieee1275.h machine/kernel.h
 
@@ -69,7 +69,7 @@ grub_emu_LDFLAGS = $(LIBCURSES)
 
 kernel_elf_SOURCES = kern/sparc64/ieee1275/init.c kern/ieee1275/ieee1275.c \
 	kern/main.c kern/device.c kern/disk.c kern/dl.c kern/file.c \
-	kern/fs.c kern/err.c kern/misc.c kern/mm.c kern/loader.c \
+	kern/fs.c kern/err.c kern/handler.c kern/misc.c kern/mm.c kern/loader.c \
 	kern/rescue.c kern/term.c term/ieee1275/ofconsole.c \
 	kern/sparc64/ieee1275/openfw.c disk/ieee1275/ofdisk.c \
 	kern/partition.c kern/env.c kern/sparc64/dl.c symlist.c \
Index: conf/i386-ieee1275.rmk
===================================================================
--- conf/i386-ieee1275.rmk	(revision 1833)
+++ conf/i386-ieee1275.rmk	(working copy)
@@ -18,7 +18,7 @@ kernel_elf_SOURCES = kern/i386/ieee1275/
 	kern/disk.c kern/dl.c kern/file.c kern/fs.c kern/err.c \
 	kern/misc.c kern/mm.c kern/loader.c kern/rescue.c kern/term.c \
 	kern/i386/dl.c kern/parser.c kern/partition.c \
-	kern/env.c \
+	kern/env.c kern/handler.c \
 	kern/time.c \
 	kern/generic/millisleep.c \
 	kern/ieee1275/ieee1275.c \
@@ -26,7 +26,7 @@ kernel_elf_SOURCES = kern/i386/ieee1275/
 	disk/ieee1275/ofdisk.c \
 	symlist.c
 kernel_elf_HEADERS = arg.h cache.h device.h disk.h dl.h elf.h elfload.h \
-	env.h err.h file.h fs.h kernel.h loader.h misc.h mm.h net.h parser.h \
+	env.h err.h file.h fs.h handler.h kernel.h loader.h misc.h mm.h net.h parser.h \
 	partition.h pc_partition.h rescue.h symbol.h term.h time.h types.h \
 	ieee1275/ieee1275.h machine/kernel.h machine/loader.h machine/memory.h
 kernel_elf_CFLAGS = $(COMMON_CFLAGS)
@@ -69,7 +69,7 @@ grub_emu_SOURCES = commands/boot.c comma
 	fs/fshelp.c							\
 	io/gzio.c							\
 	kern/device.c kern/disk.c kern/dl.c kern/elf.c kern/env.c	\
-	kern/err.c							\
+	kern/err.c kern/handler.c					\
 	normal/execute.c kern/file.c kern/fs.c normal/lexer.c 		\
 	kern/loader.c kern/main.c kern/misc.c kern/parser.c		\
 	grub_script.tab.c kern/partition.c kern/rescue.c kern/term.c	\
Index: kern/fs.c
===================================================================
--- kern/fs.c	(revision 1833)
+++ kern/fs.c	(working copy)
@@ -27,39 +27,9 @@
 #include <grub/mm.h>
 #include <grub/term.h>
 
-static grub_fs_t grub_fs_list;
-
 grub_fs_autoload_hook_t grub_fs_autoload_hook = 0;
 
-void
-grub_fs_register (grub_fs_t fs)
-{
-  fs->next = grub_fs_list;
-  grub_fs_list = fs;
-}
-
-void
-grub_fs_unregister (grub_fs_t fs)
-{
-  grub_fs_t *p, q;
-
-  for (p = &grub_fs_list, q = *p; q; p = &(q->next), q = q->next)
-    if (q == fs)
-      {
-	*p = q->next;
-	break;
-      }
-}
-
-void
-grub_fs_iterate (int (*hook) (const grub_fs_t fs))
-{
-  grub_fs_t p;
-
-  for (p = grub_fs_list; p; p = p->next)
-    if (hook (p))
-      break;
-}
+GRUB_HANDLER_CREATE_C (grub_fs);
 
 grub_fs_t
 grub_fs_probe (grub_device_t device)
Index: kern/handler.c
===================================================================
--- kern/handler.c	(revision 0)
+++ kern/handler.c	(revision 0)
@@ -0,0 +1,56 @@
+/* handler.c - handle handlers */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/types.h>
+#include <grub/misc.h>
+#include <grub/mm.h>
+#include <grub/handler.h>
+
+void
+grub_handler_register (grub_handler_t *head, grub_handler_t handler)
+{
+  handler->next = *head;
+  *head = handler;
+}
+
+void
+grub_handler_unregister (grub_handler_t *head, grub_handler_t handler)
+{
+  grub_handler_t *p, q;
+
+  for (p = head, q = *p; q; p = &(q->next), q = q->next)
+    if (q == handler)
+      {
+	*p = q->next;
+	break;
+      }
+}
+
+int
+grub_handler_iterate (grub_handler_t head,
+		      int (*hook) (const grub_handler_t handler))
+{
+  grub_handler_t p;
+
+  for (p = head; p; p = p->next)
+    if (hook (p))
+      break;
+
+  return 0;
+}
Index: genkernsyms.sh.in
===================================================================
--- genkernsyms.sh.in	(revision 1833)
+++ genkernsyms.sh.in	(working copy)
@@ -24,4 +24,5 @@ $CC -DGRUB_SYMBOL_GENERATOR=1 -E -I. -Ii
   | sed -n \
         -e '/EXPORT_FUNC *([a-zA-Z0-9_]*)/{s/.*EXPORT_FUNC *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
         -e '/EXPORT_VAR *([a-zA-Z0-9_]*)/{s/.*EXPORT_VAR *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1 kernel/;p;}' \
+        -e '/GRUB_HANDLER_CREATE_H_KERNEL *([a-zA-Z0-9_]*)/{s/.*GRUB_HANDLER_CREATE_H_KERNEL *(\([a-zA-Z0-9_]*\)).*/'"$u"'\1_register kernel\n\1_unregister kernel\n\1_iterate kernel/;p;}' \
   | sort -u
Index: include/grub/fs.h
===================================================================
--- include/grub/fs.h	(revision 1833)
+++ include/grub/fs.h	(working copy)
@@ -23,6 +23,7 @@
 #include <grub/device.h>
 #include <grub/symbol.h>
 #include <grub/types.h>
+#include <grub/handler.h>
 
 /* Forward declaration is required, because of mutual reference.  */
 struct grub_file;
@@ -30,8 +31,7 @@ struct grub_file;
 /* Filesystem descriptor.  */
 struct grub_fs
 {
-  /* My name.  */
-  const char *name;
+  GRUB_HANDLER_CREATE_STRUCT(grub_fs);
 
   /* Call HOOK with each file under DIR.  */
   grub_err_t (*dir) (grub_device_t device, const char *path,
@@ -55,11 +55,9 @@ struct grub_fs
      returned in a grub_malloc'ed buffer and should be freed by the
      caller.  */
   grub_err_t (*uuid) (grub_device_t device, char **uuid);
-
-  /* The next filesystem.  */
-  struct grub_fs *next;
 };
-typedef struct grub_fs *grub_fs_t;
+
+GRUB_HANDLER_CREATE_H_KERNEL(grub_fs);
 
 /* This is special, because block lists are not files in usual sense.  */
 extern struct grub_fs grub_fs_blocklist;
@@ -71,9 +69,6 @@ extern struct grub_fs grub_fs_blocklist;
 typedef int (*grub_fs_autoload_hook_t) (void);
 extern grub_fs_autoload_hook_t EXPORT_VAR(grub_fs_autoload_hook);
 
-void EXPORT_FUNC(grub_fs_register) (grub_fs_t fs);
-void EXPORT_FUNC(grub_fs_unregister) (grub_fs_t fs);
-void EXPORT_FUNC(grub_fs_iterate) (int (*hook) (const grub_fs_t fs));
 grub_fs_t EXPORT_FUNC(grub_fs_probe) (grub_device_t device);
 
 #endif /* ! GRUB_FS_HEADER */
Index: include/grub/handler.h
===================================================================
--- include/grub/handler.h	(revision 0)
+++ include/grub/handler.h	(revision 0)
@@ -0,0 +1,105 @@
+/* handler.h - handler interfaces */
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2008  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef GRUB_HANDLER_HEADER
+#define GRUB_HANDLER_HEADER	1
+
+struct grub_handler
+{
+  struct grub_handler *next;
+};
+typedef struct grub_handler *grub_handler_t;
+
+void grub_handler_register (grub_handler_t *head, grub_handler_t handler);
+
+void grub_handler_unregister (grub_handler_t *head, grub_handler_t handler);
+
+int grub_handler_iterate (grub_handler_t head,
+			  int (*hook) (const grub_handler_t handler));
+
+\f
+
+/* Whenever a struct that needs a handler is defined, use this macro
+   as if it is a member.  HNAME is the name of the handler, HSNAME is
+   the name of the struct the handler uses.  */
+#define GRUB_HANDLER_CREATE_STRUCT(hsname)				\
+  /* The next struct of this kind.  */					\
+  struct hsname *next;							\
+									\
+  /* My name.  */							\
+  const char *name;
+
+/* Create the types for the handler.  This macro should be used inside
+   the header file for handlers defined by the GRUB kernel.  Users of
+   this macro should define the prototypes themselves.  HSNAME is the
+   name of the struct the handler uses.  */
+#ifndef GRUB_SYMBOL_GENERATOR
+#define GRUB_HANDLER_CREATE_H_KERNEL(hsname)				\
+typedef struct hsname *hsname ## _t;					\
+extern hsname ## _t hsname ## _list;					\
+typedef int (*hsname ## _hookfunc) (const grub_handler_t handler);	\
+void hsname ## _register (hsname ## _t);				\
+void hsname ## _unregister (hsname ## _t);				\
+void hsname ##_iterate (int (*hook) (const hsname ## _t h));
+#else
+
+#endif
+
+/* Create the types and prototypes for the handler.  This macro should
+   be used inside the header file for handlers defined by GRUB
+   modules.  HSNAME is the name of the struct the handler uses.  */
+#define GRUB_HANDLER_CREATE_H(hsname)					\
+typedef struct hsname *hsname ## _t;					\
+extern hsname ## _t hsname ## _list;					\
+typedef int (*hsname ## _hookfunc) (const grub_handler_t handler);	\
+void hsname ## _register (hsname ## _t);				\
+void hsname ## _unregister (hsname ## _t);				\
+void hsname ##_iterate (int (*hook) (const hsname ## _t h));
+
+
+/* Create the wrapper functions for the handler.  This macro should be
+   used inside the C file for handlers.  HSNAME is the name of the
+   struct the handler uses.  */
+#define GRUB_HANDLER_CREATE_C(hsname)					\
+hsname ## _t hsname ## _list;						\
+									\
+void									\
+hsname ##_register (hsname ## _t handler)				\
+{									\
+  grub_handler_register ((grub_handler_t *)				\
+			 (void *) &hsname ## _list,			\
+			 (grub_handler_t) handler);			\
+}									\
+									\
+void									\
+hsname ##_unregister (hsname ## _t handler)				\
+{									\
+  grub_handler_unregister ((grub_handler_t *)				\
+			   (void *) &hsname ## _list,			\
+			   (grub_handler_t) handler);			\
+}									\
+									\
+void									\
+hsname ##_iterate (int (*hook) (const hsname ## _t h))			\
+{									\
+  grub_handler_iterate ((grub_handler_t) hsname ## _list,		\
+			(hsname ## _hookfunc) hook);			\
+}
+
+#endif /* ! GRUB_HANDLER_HEADER */

Re: Handlers

[Neal H. Walfield]

I know know why you call this a handler; it seems to me that it is
just a semi-generic list package.  Am I missing something?

You can find a slighly more flexible and generic implementation here:

  http://cvs.savannah.gnu.org/viewvc/hurd-l4/viengoos/list.h?root=hurd&view=markup

I've been using that for a while and am quite satisfied with it.

Perhaps you'll find it useful.

Neal


Two comments:

At Fri, 29 Aug 2008 14:36:56 +0200,
Marco Gerards wrote:
> +void
> +grub_handler_unregister (grub_handler_t *head, grub_handler_t handler)
> +{
> +  grub_handler_t *p, q;
> +
> +  for (p = head, q = *p; q; p = &(q->next), q = q->next)
                    ^^^^^^                     ^^^^^^^^^^^

This is a bit inconsistent.

  for (p = head, q = *head; q; p = &(q->next), q = q->next)

or

  for (p = head, q = *p; q; p = &(q->next), q = *p)


Or, more succinctly:

  for (p = head; (q = *p); p = &(q->next))


> +int
> +grub_handler_iterate (grub_handler_t head,
> +		      int (*hook) (const grub_handler_t handler))
> +{
> +  grub_handler_t p;
> +
> +  for (p = head; p; p = p->next)
> +    if (hook (p))
> +      break;
> +
> +  return 0;
> +}

A suggestion: when HOOK returns a non-zero value, return that from the
function:

     for (p = head; p; p = p->next)
       {
         int ret = hook (p);
         if (ret)
           return ret;
       }
     return 0;

Re: Handlers

[Marco Gerards]

"Neal H. Walfield" <neal@walfield.org> writes:

> I know know why you call this a handler; it seems to me that it is
> just a semi-generic list package.  Am I missing something?

Perhaps.  I can better explain how this can be used and give a small
example.  GRUB has several types of handlers.  I hope a handler is the
right word, please correct me if it's not.  Examples of handlers are
filesystems, terminals, partitioning schemes, commands, etc.  A
handler usually consists of a struct with function pointers and a
pointer to the next handler of its kind.

Let's focus on filesystems.  To implement the filesystem handler, we
defined 3 basic functions.  You need to be able to register and
unregister filesystems.  It should be possible to iterate over all
filesystems.  grub_file_open, for example, iterates over all
filesystems to see if it detects a certain filesystem layout on some
disk.

What we currently did is rewriting grub_*_register, grub_*_unregister
and grub_*_iterate.  It's boring to rewrite such functions all the
time and this results in duplicated code.

So it is not a list in the classical sense, it does not contain data.

> You can find a slighly more flexible and generic implementation here:
>
>   http://cvs.savannah.gnu.org/viewvc/hurd-l4/viengoos/list.h?root=hurd&view=markup
>
> I've been using that for a while and am quite satisfied with it.
>
> Perhaps you'll find it useful.

It certainly looks good, thanks for the suggestion.  However, I do not
think we have the same goals.  For example, I focus on size and do not
need many features.

> Two comments:
>
> At Fri, 29 Aug 2008 14:36:56 +0200,
> Marco Gerards wrote:
>> +void
>> +grub_handler_unregister (grub_handler_t *head, grub_handler_t handler)
>> +{
>> +  grub_handler_t *p, q;
>> +
>> +  for (p = head, q = *p; q; p = &(q->next), q = q->next)
>                     ^^^^^^                     ^^^^^^^^^^^
>
> This is a bit inconsistent.
>
>   for (p = head, q = *head; q; p = &(q->next), q = q->next)
>
> or
>
>   for (p = head, q = *p; q; p = &(q->next), q = *p)
>
>
> Or, more succinctly:
>
>   for (p = head; (q = *p); p = &(q->next))

You are right, I simply blindly copied this from existing code.  I can
make this more consistent.

>> +int
>> +grub_handler_iterate (grub_handler_t head,
>> +		      int (*hook) (const grub_handler_t handler))
>> +{
>> +  grub_handler_t p;
>> +
>> +  for (p = head; p; p = p->next)
>> +    if (hook (p))
>> +      break;
>> +
>> +  return 0;
>> +}
>
> A suggestion: when HOOK returns a non-zero value, return that from the
> function:
>
>      for (p = head; p; p = p->next)
>        {
>          int ret = hook (p);
>          if (ret)
>            return ret;
>        }
>      return 0;

Well spotted!  Normally we use:

if (hook (p))
  return 1;

This is what happens when you blindly copy existing code, it might not
meet the behavior you expect :-/

--
Marco

Re: Handlers

[Vesa Jääskeläinen]

Marco Gerards wrote:
> This patch adds handler support to GRUB 2.  This makes it easier to
> add new handlers to GRUB without duplicating code over and over.  Bean
> wanted this for some of his ideas :-)
> 
> Okuji (and others), can you please review this patch?  It is an
> intrusive (but yet small!) change of which I hope most people agree
> with.  I just don't want to commit this without proper review.

Hi,

Just from previous experience with macros... it dam hard to debug with
them when you are using debugger. SO... if you can avoid that please do
it that way. Can you try to think alternative version without using macros?

Thanks,
Vesa Jääskeläinen

Re: Handlers

[Marco Gerards]

Hi,

Vesa Jääskeläinen <chaac@nic.fi> writes:

> Marco Gerards wrote:
>> This patch adds handler support to GRUB 2.  This makes it easier to
>> add new handlers to GRUB without duplicating code over and over.  Bean
>> wanted this for some of his ideas :-)
>> 
>> Okuji (and others), can you please review this patch?  It is an
>> intrusive (but yet small!) change of which I hope most people agree
>> with.  I just don't want to commit this without proper review.
>
> Just from previous experience with macros... it dam hard to debug with
> them when you are using debugger. SO... if you can avoid that please do
> it that way. Can you try to think alternative version without using macros?

If I knew a better way, I would have used it.  I think the
alternatives will all end up in either code duplication or a loss of
type safety that now is at least present through warnings.  But please
understand that you only have to use the macros, not change them :-)

Suggestions are welcome.

--
Marco

Re: Handlers

[Neal H. Walfield]

At Fri, 29 Aug 2008 16:46:32 +0200,
Marco Gerards wrote:
> "Neal H. Walfield" <neal@walfield.org> writes:
> 
> > I know know why you call this a handler; it seems to me that it is
> > just a semi-generic list package.  Am I missing something?
> 
> Perhaps.  I can better explain how this can be used and give a small
> example.  GRUB has several types of handlers.  I hope a handler is the
> right word, please correct me if it's not.  Examples of handlers are
> filesystems, terminals, partitioning schemes, commands, etc.  A
> handler usually consists of a struct with function pointers and a
> pointer to the next handler of its kind.

I'd call it a factory.  See:

  http://en.wikipedia.org/wiki/Factory_object

> So it is not a list in the classical sense, it does not contain data.

What about it makes it a list in a non-classical sense?

> > You can find a slighly more flexible and generic implementation here:
> >
> >   http://cvs.savannah.gnu.org/viewvc/hurd-l4/viengoos/list.h?root=hurd&view=markup
> >
> > I've been using that for a while and am quite satisfied with it.
> >
> > Perhaps you'll find it useful.
> 
> It certainly looks good, thanks for the suggestion.  However, I do not
> think we have the same goals.  For example, I focus on size and do not
> need many features.

Sure.  I guess my concern is that lists are useful and it is also
often useful to have an object on multiple lists or to use your struct
trick in other scenarios.  If either of those might be the case, it
might be useful to borrow the list_node idea.

Neal

Re: Handlers

[Vesa Jääskeläinen]

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

Marco Gerards wrote:
> If I knew a better way, I would have used it.  I think the
> alternatives will all end up in either code duplication or a loss of
> type safety that now is at least present through warnings.  But please
> understand that you only have to use the macros, not change them :-)
> 
> Suggestions are welcome.

Hi,

Well... How about something like this?

It only requires one cast and no macros at all.

Thanks,
Vesa Jääskeläinen

[-- Attachment #2: list.c --]
[-- Type: text/plain, Size: 2051 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

typedef struct grub_list 
{
	struct grub_list *next;
} grub_list_t;

typedef struct
{
	/* Has to be first */
	grub_list_t entry;

	/* Just some data */
	char name[32];
} grub_fs_t;

void grub_list_add(grub_list_t **list, grub_list_t *item)
{
	item->next = *list;
	*list = item;
}

void grub_list_remove(grub_list_t **list, grub_list_t *item)
{
	grub_list_t *prev = 0;
	grub_list_t *tmp = *list;
	
	while (tmp)
	{
		if (tmp == item)
		{
			if (! prev)
			{
				*list = tmp->next;
				tmp->next = 0;
				return;
			}
			else
			{
				prev->next = tmp->next;
				tmp->next = 0;
				return;
			}
		}
		
		prev = tmp;
		tmp = tmp->next;
	}
}

int grub_list_iterate(grub_list_t *list, int (*hook) (const grub_list_t *item))
{
	while (list)
	{
		if (hook(list))
			return 1;

		list = list->next;
	}

	return 0;
}

int grub_fs_print (const grub_list_t *list_item)
{
	grub_fs_t *item = (grub_fs_t *)list_item;

	printf("'%s'\n", item->name);

	return 0;
}

int main()
{
	grub_fs_t item1;
	grub_fs_t item2;
	grub_fs_t item3;

	grub_list_t *fs_list = 0;

	memset(&item1, 0, sizeof(item1));
	memset(&item2, 0, sizeof(item2));
	memset(&item3, 0, sizeof(item3));

	strcpy(item1.name, "item 1");
	strcpy(item2.name, "item 2");
	strcpy(item3.name, "item 3");

	grub_list_add(&fs_list, &item1.entry);
	grub_list_add(&fs_list, &item2.entry);
	grub_list_add(&fs_list, &item3.entry);

	printf("---\n");
	grub_list_iterate(fs_list, grub_fs_print);
	
	printf("---\n");
	grub_list_remove(&fs_list, &item2.entry);
	grub_list_iterate(fs_list, grub_fs_print);

	printf("---\n");
	grub_list_remove(&fs_list, &item2.entry);
	grub_list_iterate(fs_list, grub_fs_print);

	printf("---\n");
	grub_list_remove(&fs_list, &item1.entry);
	grub_list_iterate(fs_list, grub_fs_print);

	printf("---\n");
	grub_list_remove(&fs_list, &item3.entry);
	grub_list_iterate(fs_list, grub_fs_print);

	printf("---\n");
	grub_list_remove(&fs_list, &item3.entry);
	grub_list_iterate(fs_list, grub_fs_print);
	printf("---\n");

	return 0;
}

Re: Handlers

[Marco Gerards]

Vesa Jääskeläinen <chaac@nic.fi> writes:

> Marco Gerards wrote:
>> If I knew a better way, I would have used it.  I think the
>> alternatives will all end up in either code duplication or a loss of
>> type safety that now is at least present through warnings.  But please
>> understand that you only have to use the macros, not change them :-)
>> 
>> Suggestions are welcome.
>
> Hi,
>
> Well... How about something like this?

This code does not describe structs with function pointers but structs
with data.  Furthermore, you made it very generic while you do need
casts.  For example, you have a generic iterate function.  Except for
the macros, this code is not too different.  But I used the macros to
glue the list code to the other code of GRUB, which is what you left
out.  But this is the essence of the discussion.

> It only requires one cast and no macros at all.

My code also contained generic code in kern/handler.c like your code.
I have added the macros to avoid casts.  This code will not work
directly in GRUB 2 for filesystems or whatever.  You need to cast at
some points, or even quite often.  And that is *exactly* what I would
like to avoid.

--
Marco

Re: Handlers

[Robert Millan]

On Fri, Aug 29, 2008 at 02:36:56PM +0200, Marco Gerards wrote:
> Hi,
> 
> This patch adds handler support to GRUB 2.  This makes it easier to
> add new handlers to GRUB without duplicating code over and over.  Bean
> wanted this for some of his ideas :-)
> 
> Okuji (and others), can you please review this patch?  It is an
> intrusive (but yet small!) change of which I hope most people agree
> with.  I just don't want to commit this without proper review.

Does it reduce code size (or at least, have the potential to do so)?  Would
be nice if it did.

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."

Re: Handlers

[Vesa Jääskeläinen]

Marco Gerards wrote:
> Vesa Jääskeläinen <chaac@nic.fi> writes:
> 
>> Marco Gerards wrote:
>>> If I knew a better way, I would have used it.  I think the
>>> alternatives will all end up in either code duplication or a loss of
>>> type safety that now is at least present through warnings.  But please
>>> understand that you only have to use the macros, not change them :-)
>>>
>>> Suggestions are welcome.
>> Hi,
>>
>> Well... How about something like this?
> 
> This code does not describe structs with function pointers but structs
> with data.  Furthermore, you made it very generic while you do need
> casts.  For example, you have a generic iterate function.  Except for
> the macros, this code is not too different.  But I used the macros to
> glue the list code to the other code of GRUB, which is what you left
> out.  But this is the essence of the discussion.
> 
>> It only requires one cast and no macros at all.
> 
> My code also contained generic code in kern/handler.c like your code.
> I have added the macros to avoid casts.  This code will not work
> directly in GRUB 2 for filesystems or whatever.  You need to cast at
> some points, or even quite often.  And that is *exactly* what I would
> like to avoid.

Hi,

But your code does the cast too :)... its just hidden inside macro
magic... Or did you lost track of it already as it was hidden ;) ;)

If you just paste code for GRUB_HANDLER_CREATE_C inside .c file we are
pretty much in the same functionality.

If we want to nitpick somewhere, your code assumes iterate makes a call
to int (*hook) (const grub_handler_t handler) where as actual function
pointer is something else.

Anyway... my point is. Do not hide code behind macros :). It just makes
life much harder when you are debugging or reading the code as you do
not see what is happening (and can even confuse debugger). You can do
generic register / unregister / iterate handling but keep it simple
stupid in code to make it easier to use in the end.

Thats my 2 cents on the issue.

Thanks,
Vesa Jääskeläinen