Clean up struct grub_module_header

PATCH In review Pavel Roskin 3 participants 5 messages Aug 22, 2009 – Sep 24, 2009

lore

Share & tools

lore

Pavel Roskin

author

2009-08-22 06:10 UTC #0 lore raw

PATCH 1/1 Clean up struct grub_module_header

`grub_module_header_types' should not be a field at all. Move enum outside struct.

Keep using an 8-bit integer for type', but make it unsigned. Remove all byteswapping for type', as it only has one byte.

Make size' 32-bit, as grub-mkimage already assumes. 4 gigabytes should be enough for a module. Use grub_host_to_target32() uniformly for the size' field in all utilities.

ChangeLog:

* include/grub/kernel.h (struct grub_module_header): Remove grub_module_header_types'. Make type' unsigned. Make `size' 32-bit on all platforms.
* util/elf/grub-mkimage.c (load_modules): Treat type' as an 8-bit field. Use grub_host_to_target32() for size'.
* util/i386/efi/grub-mkimage.c (make_mods_section): Likewise.
* util/i386/pc/grub-mkimage.c (generate_image): Likewise.
* util/sparc64/ieee1275/grub-mkimage.c (generate_image): Likewise.

---
 include/grub/kernel.h                |   18 +++++++++---------
 util/elf/grub-mkimage.c              |    6 +++---
 util/i386/efi/grub-mkimage.c         |    4 ++--
 util/i386/pc/grub-mkimage.c          |   12 ++++++------
 util/sparc64/ieee1275/grub-mkimage.c |    8 ++++----
 5 files changed, 24 insertions(+), 24 deletions(-)

include/grub/kernel.h +9 -9

index 02bc276..75ec77c 100644
@@ -22,20 +22,20 @@
 #include <grub/types.h>
 #include <grub/symbol.h>
 
+enum
+{
+  OBJ_TYPE_ELF,
+  OBJ_TYPE_MEMDISK,
+  OBJ_TYPE_CONFIG
+};
+
 /* The module header.  */
 struct grub_module_header
 {
   /* The type of object.  */
-  grub_int8_t type;
-  enum
-  {
-    OBJ_TYPE_ELF,
-    OBJ_TYPE_MEMDISK,
-    OBJ_TYPE_CONFIG
-  }  grub_module_header_types;
-
+  grub_uint8_t type;
   /* The size of object (including this header).  */
-  grub_target_size_t size;
+  grub_uint32_t size;
 };
 
 /* "gmim" (GRUB Module Info Magic).  */

util/elf/grub-mkimage.c +3 -3

index 52bb639..5354272 100644
@@ -144,7 +144,7 @@ load_modules (grub_addr_t modbase, Elf32_Phdr *phdr, const char *dir,
       mod_size = grub_util_get_image_size (p->name);
 
       header = (struct grub_module_header *) (module_img + offset);
-      header->type = grub_host_to_target32 (OBJ_TYPE_ELF);
+      header->type = OBJ_TYPE_ELF;
       header->size = grub_host_to_target32 (mod_size + sizeof (*header));
 
       grub_util_load_image (p->name, module_img + offset + sizeof (*header));

@@ -157,8 +157,8 @@ load_modules (grub_addr_t modbase, Elf32_Phdr *phdr, const char *dir,
       struct grub_module_header *header;
 
       header = (struct grub_module_header *) (module_img + offset);
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_MEMDISK);
-      header->size = grub_cpu_to_le32 (memdisk_size + sizeof (*header));
+      header->type = OBJ_TYPE_MEMDISK;
+      header->size = grub_host_to_target32 (memdisk_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (memdisk_path, module_img + offset);

util/i386/efi/grub-mkimage.c +2 -2

index 466aa9f..29a823e 100644
@@ -683,8 +683,8 @@ make_mods_section (FILE *out, Elf_Addr current_address,
       grub_util_info ("adding module %s", p->name);
 
       mod_size = grub_util_get_image_size (p->name);
-      header.type = grub_cpu_to_le32 (OBJ_TYPE_ELF);
-      header.size = grub_cpu_to_le32 (mod_size + sizeof (header));
+      header.type = OBJ_TYPE_ELF;
+      header.size = grub_host_to_target32 (mod_size + sizeof (header));
 
       mod_image = grub_util_read_image (p->name);

util/i386/pc/grub-mkimage.c +6 -6

index b81392c..cf8f8f5 100644
@@ -153,8 +153,8 @@ generate_image (const char *dir, char *prefix, FILE *out, char *mods[],
 
       header = (struct grub_module_header *) (kernel_img + offset);
       memset (header, 0, sizeof (struct grub_module_header));
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_ELF);
-      header->size = grub_cpu_to_le32 (mod_size + sizeof (*header));
+      header->type = OBJ_TYPE_ELF;
+      header->size = grub_host_to_target32 (mod_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (p->name, kernel_img + offset);

@@ -167,8 +167,8 @@ generate_image (const char *dir, char *prefix, FILE *out, char *mods[],
 
       header = (struct grub_module_header *) (kernel_img + offset);
       memset (header, 0, sizeof (struct grub_module_header));
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_MEMDISK);
-      header->size = grub_cpu_to_le32 (memdisk_size + sizeof (*header));
+      header->type = OBJ_TYPE_MEMDISK;
+      header->size = grub_host_to_target32 (memdisk_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (memdisk_path, kernel_img + offset);

@@ -181,8 +181,8 @@ generate_image (const char *dir, char *prefix, FILE *out, char *mods[],
 
       header = (struct grub_module_header *) (kernel_img + offset);
       memset (header, 0, sizeof (struct grub_module_header));
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_CONFIG);
-      header->size = grub_cpu_to_le32 (config_size + sizeof (*header));
+      header->type = OBJ_TYPE_CONFIG;
+      header->size = grub_host_to_target32 (config_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (config_path, kernel_img + offset);

util/sparc64/ieee1275/grub-mkimage.c +4 -4

index 247f138..0a611da 100644
@@ -98,8 +98,8 @@ generate_image (const char *dir, const char *prefix, FILE *out, char *mods[], ch
       mod_size = grub_util_get_image_size (p->name);
 
       header = (struct grub_module_header *) (kernel_img + offset);
-      header->type = grub_cpu_to_be32 (OBJ_TYPE_ELF);
-      header->size = grub_cpu_to_be32 (mod_size + sizeof (*header));
+      header->type = OBJ_TYPE_ELF;
+      header->size = grub_host_to_target32 (mod_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (p->name, kernel_img + offset);

@@ -111,8 +111,8 @@ generate_image (const char *dir, const char *prefix, FILE *out, char *mods[], ch
       struct grub_module_header *header;
 
       header = (struct grub_module_header *) (kernel_img + offset);
-      header->type = grub_cpu_to_be32 (OBJ_TYPE_MEMDISK);
-      header->size = grub_cpu_to_be32 (memdisk_size + sizeof (*header));
+      header->type = OBJ_TYPE_MEMDISK;
+      header->size = grub_host_to_target32 (memdisk_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (memdisk_path, kernel_img + offset);

Robert Millan

2009-08-23 10:50 UTC #1 lore raw

include/grub/kernel.h line 30

Quoted 6 lines · Pavel Roskin

include/grub/kernel.h +6 -0

+enum
+{
+  OBJ_TYPE_ELF,
+  OBJ_TYPE_MEMDISK,
+  OBJ_TYPE_CONFIG
+};

Do we actually use OBJ_TYPE_CONFIG ? I think it's there in the framework, but we don't provide any means to use it. Instead, grub-mkrescue puts grub.cfg in the memdisk filesystem.

Does it even make sense to support this object type? AFAICT the situation has been like this for a long while and nobody complained.

Signature · 5 lines

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."

Vladimir 'phcoder' Serbinenko

2009-08-23 11:08 UTC #2 lore raw

Quoted 12 lines · Robert Millan

include/grub/kernel.h line 30
Quoted 6 lines · Pavel Roskin
include/grub/kernel.h +6 -0
+enum
+{
+  OBJ_TYPE_ELF,
+  OBJ_TYPE_MEMDISK,
+  OBJ_TYPE_CONFIG
+};
Do we actually use OBJ_TYPE_CONFIG ? I think it's there in the framework, but we don't provide any means to use it. Instead, grub-mkrescue puts grub.cfg in the memdisk filesystem.

Quoted 3 lines

Does it even make sense to support this object type? AFAICT the situation has been like this for a long while and nobody complained.

Actually OBJ_TYPE_CONFIG is useful and it provides a way to execute a file with rescue parser before other parsers are available. It's useful to find root in special configurations. One of my TODO items is to replace (UUID=...)/ syntax with using search command based on this feature

Quoted 13 lines

--
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."
_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 4 lines

Regards
Vladimir 'phcoder' Serbinenko

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

Robert Millan

2009-08-23 23:11 UTC #3 lore raw

Quoted 13 lines · Vladimir 'phcoder' Serbinenko

Quoted 12 lines · Robert Millan
include/grub/kernel.h line 30
Quoted 6 lines · Pavel Roskin
include/grub/kernel.h +6 -0
+enum
+{
+  OBJ_TYPE_ELF,
+  OBJ_TYPE_MEMDISK,
+  OBJ_TYPE_CONFIG
+};
Do we actually use OBJ_TYPE_CONFIG ? I think it's there in the framework, but we don't provide any means to use it. Instead, grub-mkrescue puts grub.cfg in the memdisk filesystem.

Quoted 8 lines

Quoted 3 lines
Does it even make sense to support this object type? AFAICT the situation has been like this for a long while and nobody complained.
Actually OBJ_TYPE_CONFIG is useful and it provides a way to execute a file with rescue parser before other parsers are available. It's useful to find root in special configurations. One of my TODO items is to replace (UUID=...)/ syntax with using search command based on this feature

Ok. I'm not very fond of the (UUID=...) hack either (yeah, I guess I'm the one to blame for it).

Signature · 5 lines

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."

Robert Millan

2009-09-24 13:41 UTC #4 lore raw

In reply to Pavel Roskin

It seems this has been leftover, probably because my response looked like I objected to it (which I don't, it's perfectly reasonable cleanup).

I'm committing it.

Quoted 171 lines · Pavel Roskin

`grub_module_header_types' should not be a field at all. Move enum outside struct.

Keep using an 8-bit integer for type', but make it unsigned. Remove all byteswapping for type', as it only has one byte.

Make size' 32-bit, as grub-mkimage already assumes. 4 gigabytes should be enough for a module. Use grub_host_to_target32() uniformly for the size' field in all utilities.

ChangeLog:

---
 include/grub/kernel.h                |   18 +++++++++---------
 util/elf/grub-mkimage.c              |    6 +++---
 util/i386/efi/grub-mkimage.c         |    4 ++--
 util/i386/pc/grub-mkimage.c          |   12 ++++++------
 util/sparc64/ieee1275/grub-mkimage.c |    8 ++++----
 5 files changed, 24 insertions(+), 24 deletions(-)

include/grub/kernel.h +9 -9

index 02bc276..75ec77c 100644
@@ -22,20 +22,20 @@
 #include <grub/types.h>
 #include <grub/symbol.h>
 
+enum
+{
+  OBJ_TYPE_ELF,
+  OBJ_TYPE_MEMDISK,
+  OBJ_TYPE_CONFIG
+};
+
 /* The module header.  */
 struct grub_module_header
 {
   /* The type of object.  */
-  grub_int8_t type;
-  enum
-  {
-    OBJ_TYPE_ELF,
-    OBJ_TYPE_MEMDISK,
-    OBJ_TYPE_CONFIG
-  }  grub_module_header_types;
-
+  grub_uint8_t type;
   /* The size of object (including this header).  */
-  grub_target_size_t size;
+  grub_uint32_t size;
 };
 
 /* "gmim" (GRUB Module Info Magic).  */

util/elf/grub-mkimage.c +3 -3

index 52bb639..5354272 100644
@@ -144,7 +144,7 @@ load_modules (grub_addr_t modbase, Elf32_Phdr *phdr, const char *dir,
       mod_size = grub_util_get_image_size (p->name);
 
       header = (struct grub_module_header *) (module_img + offset);
-      header->type = grub_host_to_target32 (OBJ_TYPE_ELF);
+      header->type = OBJ_TYPE_ELF;
       header->size = grub_host_to_target32 (mod_size + sizeof (*header));
 
       grub_util_load_image (p->name, module_img + offset + sizeof (*header));

@@ -157,8 +157,8 @@ load_modules (grub_addr_t modbase, Elf32_Phdr *phdr, const char *dir,
       struct grub_module_header *header;
 
       header = (struct grub_module_header *) (module_img + offset);
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_MEMDISK);
-      header->size = grub_cpu_to_le32 (memdisk_size + sizeof (*header));
+      header->type = OBJ_TYPE_MEMDISK;
+      header->size = grub_host_to_target32 (memdisk_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (memdisk_path, module_img + offset);

util/i386/efi/grub-mkimage.c +2 -2

index 466aa9f..29a823e 100644
@@ -683,8 +683,8 @@ make_mods_section (FILE *out, Elf_Addr current_address,
       grub_util_info ("adding module %s", p->name);
 
       mod_size = grub_util_get_image_size (p->name);
-      header.type = grub_cpu_to_le32 (OBJ_TYPE_ELF);
-      header.size = grub_cpu_to_le32 (mod_size + sizeof (header));
+      header.type = OBJ_TYPE_ELF;
+      header.size = grub_host_to_target32 (mod_size + sizeof (header));
 
       mod_image = grub_util_read_image (p->name);

util/i386/pc/grub-mkimage.c +6 -6

index b81392c..cf8f8f5 100644
@@ -153,8 +153,8 @@ generate_image (const char *dir, char *prefix, FILE *out, char *mods[],
 
       header = (struct grub_module_header *) (kernel_img + offset);
       memset (header, 0, sizeof (struct grub_module_header));
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_ELF);
-      header->size = grub_cpu_to_le32 (mod_size + sizeof (*header));
+      header->type = OBJ_TYPE_ELF;
+      header->size = grub_host_to_target32 (mod_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (p->name, kernel_img + offset);

@@ -167,8 +167,8 @@ generate_image (const char *dir, char *prefix, FILE *out, char *mods[],
 
       header = (struct grub_module_header *) (kernel_img + offset);
       memset (header, 0, sizeof (struct grub_module_header));
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_MEMDISK);
-      header->size = grub_cpu_to_le32 (memdisk_size + sizeof (*header));
+      header->type = OBJ_TYPE_MEMDISK;
+      header->size = grub_host_to_target32 (memdisk_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (memdisk_path, kernel_img + offset);

@@ -181,8 +181,8 @@ generate_image (const char *dir, char *prefix, FILE *out, char *mods[],
 
       header = (struct grub_module_header *) (kernel_img + offset);
       memset (header, 0, sizeof (struct grub_module_header));
-      header->type = grub_cpu_to_le32 (OBJ_TYPE_CONFIG);
-      header->size = grub_cpu_to_le32 (config_size + sizeof (*header));
+      header->type = OBJ_TYPE_CONFIG;
+      header->size = grub_host_to_target32 (config_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (config_path, kernel_img + offset);

util/sparc64/ieee1275/grub-mkimage.c +4 -4

index 247f138..0a611da 100644
@@ -98,8 +98,8 @@ generate_image (const char *dir, const char *prefix, FILE *out, char *mods[], ch
       mod_size = grub_util_get_image_size (p->name);
 
       header = (struct grub_module_header *) (kernel_img + offset);
-      header->type = grub_cpu_to_be32 (OBJ_TYPE_ELF);
-      header->size = grub_cpu_to_be32 (mod_size + sizeof (*header));
+      header->type = OBJ_TYPE_ELF;
+      header->size = grub_host_to_target32 (mod_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (p->name, kernel_img + offset);

@@ -111,8 +111,8 @@ generate_image (const char *dir, const char *prefix, FILE *out, char *mods[], ch
       struct grub_module_header *header;
 
       header = (struct grub_module_header *) (kernel_img + offset);
-      header->type = grub_cpu_to_be32 (OBJ_TYPE_MEMDISK);
-      header->size = grub_cpu_to_be32 (memdisk_size + sizeof (*header));
+      header->type = OBJ_TYPE_MEMDISK;
+      header->size = grub_host_to_target32 (memdisk_size + sizeof (*header));
       offset += sizeof (*header);
 
       grub_util_load_image (memdisk_path, kernel_img + offset);

_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel

Signature · 5 lines

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."