[RFC PATCH] Add new partition type API and convert GPT type checks.

[RFC PATCH] Add new partition type API and convert GPT type checks.

[Michael Marineau]

This introduces a new type() function for partmaps modeled after the
label() and uuid() functions for filesystems. A likely future extension
will be support partition labels and uuids as well. This is in
preparation for adding more functionality like a search.part_type
command.

The msdos partition code is only partially converted for now, I'm not
sure about the preferred way to do this. Re-reading a msdos partition
table is not as easy as re-reading gpt is. One option would be to turn
'msdostype' into a union or data pointer that partmap modules can stash
extra info in. That would also allow gpt to avoid re-reading. Being new
to grub, are there memory usage concerns I should be aware of here?
---
 grub-core/disk/ldm.c         | 17 ++----------
 grub-core/kern/partition.c   | 23 +++++++++++++++++
 grub-core/partmap/gpt.c      | 61 ++++++++++++++++++++++++++++++--------------
 grub-core/partmap/msdos.c    |  9 +++++++
 include/grub/gpt_partition.h | 20 +++------------
 include/grub/partition.h     | 11 ++++++++
 util/grub-install.c          | 34 ++++--------------------
 util/grub-probe.c            | 54 +++++++++++++++------------------------
 8 files changed, 117 insertions(+), 112 deletions(-)

diff --git a/grub-core/disk/ldm.c b/grub-core/disk/ldm.c
index 8075f2a..6dcfbc8 100644
--- a/grub-core/disk/ldm.c
+++ b/grub-core/disk/ldm.c
@@ -135,31 +135,18 @@ msdos_has_ldm_partition (grub_disk_t dsk)
   return has_ldm;
 }
 
-static const grub_gpt_part_type_t ldm_type = GRUB_GPT_PARTITION_TYPE_LDM;
-
 /* Helper for gpt_ldm_sector.  */
 static int
 gpt_ldm_sector_iter (grub_disk_t disk, const grub_partition_t p, void *data)
 {
   grub_disk_addr_t *sector = data;
-  struct grub_gpt_partentry gptdata;
-  grub_partition_t p2;
-
-  p2 = disk->partition;
-  disk->partition = p->parent;
-  if (grub_disk_read (disk, p->offset, p->index,
-		      sizeof (gptdata), &gptdata))
-    {
-      disk->partition = p2;
-      return 0;
-    }
-  disk->partition = p2;
 
-  if (! grub_memcmp (&gptdata.type, &ldm_type, 16))
+  if (grub_partition_is_type (disk, p, "gpt", GRUB_GPT_PARTITION_TYPE_LDM))
     {
       *sector = p->start + p->len - 1;
       return 1;
     }
+
   return 0;
 }
 
diff --git a/grub-core/kern/partition.c b/grub-core/kern/partition.c
index e499147..f27a8bd 100644
--- a/grub-core/kern/partition.c
+++ b/grub-core/kern/partition.c
@@ -274,3 +274,26 @@ grub_partition_get_name (const grub_partition_t partition)
   grub_memmove (out, ptr + 1, out + needlen - ptr);
   return out;
 }
+
+int grub_partition_is_type (struct grub_disk *disk,
+			    const grub_partition_t partition,
+			    const char *partmap_name,
+			    const char *partition_type)
+{
+  char *type;
+  int r;
+
+  if (!disk || !partition || !partition->partmap->type)
+    return 0;
+
+  if (grub_strcmp (partition->partmap->name, partmap_name) != 0)
+    return 0;
+
+  if (partition->partmap->type(disk, partition, &type) != 0)
+    return 0;
+
+  r = grub_strcmp (type, partition_type);
+  grub_free (type);
+
+  return r == 0;
+}
diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
index 38df7b3..6d6c13f 100644
--- a/grub-core/partmap/gpt.c
+++ b/grub-core/partmap/gpt.c
@@ -33,11 +33,10 @@ static grub_uint8_t grub_gpt_magic[8] =
     0x45, 0x46, 0x49, 0x20, 0x50, 0x41, 0x52, 0x54
   };
 
-static const grub_gpt_part_type_t grub_gpt_partition_type_empty = GRUB_GPT_PARTITION_TYPE_EMPTY;
-
-#ifdef GRUB_UTIL
-static const grub_gpt_part_type_t grub_gpt_partition_type_bios_boot = GRUB_GPT_PARTITION_TYPE_BIOS_BOOT;
-#endif
+static const grub_gpt_part_type_t grub_gpt_partition_type_empty =
+  { 0x0, 0x0, 0x0,
+    { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
+  };
 
 /* 512 << 7 = 65536 byte sectors.  */
 #define MAX_SECTOR_LOG 7
@@ -128,6 +127,40 @@ grub_gpt_partition_map_iterate (grub_disk_t disk,
   return GRUB_ERR_NONE;
 }
 
+static grub_err_t
+grub_gpt_partition_type (grub_disk_t disk, const grub_partition_t partition,
+			 char **type)
+{
+  struct grub_gpt_partentry gptentry;
+  grub_gpt_part_type_t gpttype;
+  grub_partition_t p2;
+  grub_err_t err;
+
+  p2 = disk->partition;
+  disk->partition = partition->parent;
+  err = grub_disk_read (disk, partition->offset, partition->index,
+			sizeof (gptentry), &gptentry);
+  disk->partition = p2;
+
+  if (err)
+    return err;
+
+  gpttype.data1 = grub_le_to_cpu32 (gptentry.type.data1);
+  gpttype.data2 = grub_le_to_cpu16 (gptentry.type.data2);
+  gpttype.data3 = grub_le_to_cpu16 (gptentry.type.data3);
+  grub_memcpy (gpttype.data4, gptentry.type.data4, sizeof (gpttype.data4));
+
+  *type = grub_xasprintf ("%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
+			  gpttype.data1, gpttype.data2,
+			  gpttype.data3, gpttype.data4[0],
+			  gpttype.data4[1], gpttype.data4[2],
+			  gpttype.data4[3], gpttype.data4[4],
+			  gpttype.data4[5], gpttype.data4[6],
+			  gpttype.data4[7]);
+
+  return GRUB_ERR_NONE;
+}
+
 #ifdef GRUB_UTIL
 /* Context for gpt_partition_map_embed.  */
 struct gpt_partition_map_embed_ctx
@@ -137,25 +170,14 @@ struct gpt_partition_map_embed_ctx
 
 /* Helper for gpt_partition_map_embed.  */
 static int
-find_usable_region (grub_disk_t disk __attribute__ ((unused)),
+find_usable_region (grub_disk_t disk,
 		    const grub_partition_t p, void *data)
 {
   struct gpt_partition_map_embed_ctx *ctx = data;
-  struct grub_gpt_partentry gptdata;
-  grub_partition_t p2;
-
-  p2 = disk->partition;
-  disk->partition = p->parent;
-  if (grub_disk_read (disk, p->offset, p->index,
-		      sizeof (gptdata), &gptdata))
-    {
-      disk->partition = p2;
-      return 0;
-    }
-  disk->partition = p2;
 
   /* If there's an embed region, it is in a dedicated partition.  */
-  if (! grub_memcmp (&gptdata.type, &grub_gpt_partition_type_bios_boot, 16))
+  if (grub_partition_is_type(disk, p, "gpt",
+			     GRUB_GPT_PARTITION_TYPE_BIOS_BOOT))
     {
       ctx->start = p->start;
       ctx->len = p->len;
@@ -215,6 +237,7 @@ static struct grub_partition_map grub_gpt_partition_map =
   {
     .name = "gpt",
     .iterate = grub_gpt_partition_map_iterate,
+    .type = grub_gpt_partition_type,
 #ifdef GRUB_UTIL
     .embed = gpt_partition_map_embed
 #endif
diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
index 1d81a53..68140a7 100644
--- a/grub-core/partmap/msdos.c
+++ b/grub-core/partmap/msdos.c
@@ -228,6 +228,14 @@ grub_partition_msdos_iterate (grub_disk_t disk,
   return grub_errno;
 }
 
+static grub_err_t
+grub_partition_msdos_type (grub_disk_t disk __attribute__ ((unused)),
+			   const grub_partition_t partition, char **type)
+{
+  *type = grub_xasprintf ("%02x", partition->msdostype);
+  return GRUB_ERR_NONE;
+}
+
 #ifdef GRUB_UTIL
 
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
@@ -415,6 +423,7 @@ static struct grub_partition_map grub_msdos_partition_map =
   {
     .name = "msdos",
     .iterate = grub_partition_msdos_iterate,
+    .type = grub_partition_msdos_type,
 #ifdef GRUB_UTIL
     .embed = pc_partition_map_embed
 #endif
diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h
index 1b32f67..566334c 100644
--- a/include/grub/gpt_partition.h
+++ b/include/grub/gpt_partition.h
@@ -31,24 +31,12 @@ struct grub_gpt_part_type
 } __attribute__ ((aligned(8)));
 typedef struct grub_gpt_part_type grub_gpt_part_type_t;
 
-#define GRUB_GPT_PARTITION_TYPE_EMPTY \
-  { 0x0, 0x0, 0x0, \
-    { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } \
-  }
-
 #define GRUB_GPT_PARTITION_TYPE_BIOS_BOOT \
-  { grub_cpu_to_le32_compile_time (0x21686148), \
-      grub_cpu_to_le16_compile_time (0x6449), \
-      grub_cpu_to_le16_compile_time (0x6e6f),	       \
-    { 0x74, 0x4e, 0x65, 0x65, 0x64, 0x45, 0x46, 0x49 } \
-  }
-
+  "21686148-6449-6e6f-744e-656564454649"
 #define GRUB_GPT_PARTITION_TYPE_LDM \
-  { grub_cpu_to_le32_compile_time (0x5808C8AAU),\
-      grub_cpu_to_le16_compile_time (0x7E8F), \
-      grub_cpu_to_le16_compile_time (0x42E0),	       \
-	{ 0x85, 0xD2, 0xE1, 0xE9, 0x04, 0x34, 0xCF, 0xB3 }	\
-  }
+  "5808c8aa-7e8f-42e0-85d2-e1e90434cfb3"
+#define GRUB_GPT_PARTITION_TYPE_PREP \
+  "9e1a2d38-c612-4316-aa26-8b49521e5a8b"
 
 struct grub_gpt_header
 {
diff --git a/include/grub/partition.h b/include/grub/partition.h
index 7adb7ec..3dc46bf 100644
--- a/include/grub/partition.h
+++ b/include/grub/partition.h
@@ -50,6 +50,13 @@ struct grub_partition_map
   /* Call HOOK with each partition, until HOOK returns non-zero.  */
   grub_err_t (*iterate) (struct grub_disk *disk,
 			 grub_partition_iterate_hook_t hook, void *hook_data);
+
+  /* Return the type of the partition PARTITION in TYPE.
+     The type is returned in a grub_malloc'ed buffer and should
+     be freed by the caller.  */
+  grub_err_t (*type) (struct grub_disk *disk,
+		      const grub_partition_t partition, char **type);
+
 #ifdef GRUB_UTIL
   /* Determine sectors available for embedding.  */
   grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
@@ -95,6 +102,10 @@ int EXPORT_FUNC(grub_partition_iterate) (struct grub_disk *disk,
 					 grub_partition_iterate_hook_t hook,
 					 void *hook_data);
 char *EXPORT_FUNC(grub_partition_get_name) (const grub_partition_t partition);
+int EXPORT_FUNC(grub_partition_is_type) (struct grub_disk *disk,
+					 const grub_partition_t partition,
+					 const char *partmap_name,
+					 const char *partition_type);
 
 
 extern grub_partition_map_t EXPORT_VAR(grub_partition_map_list);
diff --git a/util/grub-install.c b/util/grub-install.c
index 7d61c32..ee98b70 100644
--- a/util/grub-install.c
+++ b/util/grub-install.c
@@ -696,36 +696,12 @@ write_to_disk (grub_device_t dev, const char *fn)
 static int
 is_prep_partition (grub_device_t dev)
 {
-  if (!dev->disk)
-    return 0;
-  if (!dev->disk->partition)
-    return 0;
-  if (strcmp(dev->disk->partition->partmap->name, "msdos") == 0)
-    return (dev->disk->partition->msdostype == 0x41);
-
-  if (strcmp (dev->disk->partition->partmap->name, "gpt") == 0)
-    {
-      struct grub_gpt_partentry gptdata;
-      grub_partition_t p = dev->disk->partition;
-      int ret = 0;
-      dev->disk->partition = dev->disk->partition->parent;
+  if (grub_partition_is_type (dev->disk, dev->disk->partition, "msdos", "41"))
+    return 1;
 
-      if (grub_disk_read (dev->disk, p->offset, p->index,
-			  sizeof (gptdata), &gptdata) == 0)
-	{
-	  const grub_gpt_part_type_t template = {
-	    grub_cpu_to_le32_compile_time (0x9e1a2d38),
-	    grub_cpu_to_le16_compile_time (0xc612),
-	    grub_cpu_to_le16_compile_time (0x4316),
-	    { 0xaa, 0x26, 0x8b, 0x49, 0x52, 0x1e, 0x5a, 0x8b }
-	  };
-
-	  ret = grub_memcmp (&template, &gptdata.type,
-			     sizeof (template)) == 0;
-	}
-      dev->disk->partition = p;
-      return ret;
-    }
+  if (grub_partition_is_type (dev->disk, dev->disk->partition,
+			      "gpt", GRUB_GPT_PARTITION_TYPE_PREP))
+    return 1;
 
   return 0;
 }
diff --git a/util/grub-probe.c b/util/grub-probe.c
index ecb7b6b..a2505f0 100644
--- a/util/grub-probe.c
+++ b/util/grub-probe.c
@@ -27,7 +27,6 @@
 #include <grub/fs.h>
 #include <grub/partition.h>
 #include <grub/msdos_partition.h>
-#include <grub/gpt_partition.h>
 #include <grub/emu/hostdisk.h>
 #include <grub/emu/getroot.h>
 #include <grub/term.h>
@@ -253,6 +252,25 @@ probe_abstraction (grub_disk_t disk, char delim)
 }
 
 static void
+probe_partition_type(grub_disk_t disk, const char *partmap, char delim)
+{
+  char *type;
+
+  if (!disk->partition || !disk->partition->partmap->type ||
+      strcmp(disk->partition->partmap->name, partmap) != 0)
+    {
+      putchar (delim);
+      return;
+    }
+
+  if (disk->partition->partmap->type(disk, disk->partition, &type))
+    grub_util_error ("%s", grub_errmsg);
+
+  printf ("%s%c", type, delim);
+  free(type);
+}
+
+static void
 probe (const char *path, char **device_names, char delim)
 {
   char **drives_names = NULL;
@@ -653,44 +671,14 @@ probe (const char *path, char **device_names, char delim)
 
       if (print == PRINT_MSDOS_PARTTYPE)
 	{
-	  if (dev->disk->partition
-	      && strcmp(dev->disk->partition->partmap->name, "msdos") == 0)
-	    printf ("%02x", dev->disk->partition->msdostype);
-
-	  putchar (delim);
+	  probe_partition_type(dev->disk, "msdos", delim);
 	  grub_device_close (dev);
 	  continue;
 	}
 
       if (print == PRINT_GPT_PARTTYPE)
 	{
-          if (dev->disk->partition
-	      && strcmp (dev->disk->partition->partmap->name, "gpt") == 0)
-            {
-              struct grub_gpt_partentry gptdata;
-              grub_partition_t p = dev->disk->partition;
-              dev->disk->partition = dev->disk->partition->parent;
-
-              if (grub_disk_read (dev->disk, p->offset, p->index,
-                                  sizeof (gptdata), &gptdata) == 0)
-                {
-                  grub_gpt_part_type_t gpttype;
-                  gpttype.data1 = grub_le_to_cpu32 (gptdata.type.data1);
-                  gpttype.data2 = grub_le_to_cpu16 (gptdata.type.data2);
-                  gpttype.data3 = grub_le_to_cpu16 (gptdata.type.data3);
-                  grub_memcpy (gpttype.data4, gptdata.type.data4, 8);
-
-                  grub_printf ("%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
-                               gpttype.data1, gpttype.data2,
-                               gpttype.data3, gpttype.data4[0], 
-                               gpttype.data4[1], gpttype.data4[2],
-                               gpttype.data4[3], gpttype.data4[4],
-                               gpttype.data4[5], gpttype.data4[6],
-                               gpttype.data4[7]);
-                }
-              dev->disk->partition = p;
-            }
-          putchar (delim);
+	  probe_partition_type(dev->disk, "gpt", delim);
           grub_device_close (dev);
           continue;
         }
-- 
1.8.5.5

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Andrei Borzenkov]

В Mon, 22 Sep 2014 11:59:06 -0700
Michael Marineau <michael.marineau@coreos.com> пишет:

> This introduces a new type() function for partmaps modeled after the
> label() and uuid() functions for filesystems. A likely future extension
> will be support partition labels and uuids as well.

Is there other partition type beyond GPT that supports it? What
advantages it has over using filesystem label or uuid?

>                                                    This is in
> preparation for adding more functionality like a search.part_type
> command.
> 

What is use case? search is expected to return a single object; but you
cannot presume uniqueness of partition of given type. That's different
from label or uuid.

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Michael Marineau]

On Mon, Sep 22, 2014 at 12:19 PM, Andrei Borzenkov <arvidjaar@gmail.com> wrote:
> В Mon, 22 Sep 2014 11:59:06 -0700
> Michael Marineau <michael.marineau@coreos.com> пишет:
>
>> This introduces a new type() function for partmaps modeled after the
>> label() and uuid() functions for filesystems. A likely future extension
>> will be support partition labels and uuids as well.
>
> Is there other partition type beyond GPT that supports it? What
> advantages it has over using filesystem label or uuid?

From a quick glance through the other partmap modules looks like amiga
has parttype and apple has a parttype and partname. msdos of course
just has one byte types. As far as I can tell it is only GPT that has
all three: type, label, and uuid. I've started with just type since
that is what I need to make use of and copying the code to re-read the
type field yet again felt silly. My final goal is actually a module
that will search for two partitions of the same type and select
between them based on some other metadata. Since searching by
partition type is already a common task in the grub code base
providing an API to do so seemed like a good starting point. Others
will likely find extending this to support label and uuid more useful.
For example with GPT the Linux kernel can use
"root=PARTUUID=<someuuid>" directly, unlike LABEL= or UUID= which must
be implemented in an initrd.

>
>>                                                    This is in
>> preparation for adding more functionality like a search.part_type
>> command.
>>
>
> What is use case? search is expected to return a single object; but you
> cannot presume uniqueness of partition of given type. That's different
> from label or uuid.

search.part_type is really just a useful test case for me but it could
be useful for finding the EFI System partition and similar special
boot partitions. It will be easy to extend to support
search.part_label or search.part_uuid which would be useful when
paired with root=PARTUUID= or when the filesystem information is
unknown or not significant.

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 22.09.2014 20:59, Michael Marineau wrote:
> This introduces a new type() function for partmaps modeled after the
> label() and uuid() functions for filesystems. A likely future extension
> will be support partition labels and uuids as well. This is in
> preparation for adding more functionality like a search.part_type
> command.
> 
> The msdos partition code is only partially converted for now, I'm not
> sure about the preferred way to do this. Re-reading a msdos partition
> table is not as easy as re-reading gpt is. One option would be to turn
> 'msdostype' into a union or data pointer that partmap modules can stash
> extra info in. That would also allow gpt to avoid re-reading. Being new
> to grub, are there memory usage concerns I should be aware of here?
Please provide a usecase. Every new feature needs a usecase. This patch
increases core size and will not go in without a damn good usecase and
justification why it needs to be done in core.
> ---
>  grub-core/disk/ldm.c         | 17 ++----------
>  grub-core/kern/partition.c   | 23 +++++++++++++++++
>  grub-core/partmap/gpt.c      | 61 ++++++++++++++++++++++++++++++--------------
>  grub-core/partmap/msdos.c    |  9 +++++++
>  include/grub/gpt_partition.h | 20 +++------------
>  include/grub/partition.h     | 11 ++++++++
>  util/grub-install.c          | 34 ++++--------------------
>  util/grub-probe.c            | 54 +++++++++++++++------------------------
>  8 files changed, 117 insertions(+), 112 deletions(-)
> 
> diff --git a/grub-core/disk/ldm.c b/grub-core/disk/ldm.c
> index 8075f2a..6dcfbc8 100644
> --- a/grub-core/disk/ldm.c
> +++ b/grub-core/disk/ldm.c
> @@ -135,31 +135,18 @@ msdos_has_ldm_partition (grub_disk_t dsk)
>    return has_ldm;
>  }
>  
> -static const grub_gpt_part_type_t ldm_type = GRUB_GPT_PARTITION_TYPE_LDM;
> -
>  /* Helper for gpt_ldm_sector.  */
>  static int
>  gpt_ldm_sector_iter (grub_disk_t disk, const grub_partition_t p, void *data)
>  {
>    grub_disk_addr_t *sector = data;
> -  struct grub_gpt_partentry gptdata;
> -  grub_partition_t p2;
> -
> -  p2 = disk->partition;
> -  disk->partition = p->parent;
> -  if (grub_disk_read (disk, p->offset, p->index,
> -		      sizeof (gptdata), &gptdata))
> -    {
> -      disk->partition = p2;
> -      return 0;
> -    }
> -  disk->partition = p2;
>  
> -  if (! grub_memcmp (&gptdata.type, &ldm_type, 16))
> +  if (grub_partition_is_type (disk, p, "gpt", GRUB_GPT_PARTITION_TYPE_LDM))
>      {
>        *sector = p->start + p->len - 1;
>        return 1;
>      }
> +
>    return 0;
>  }
>  
> diff --git a/grub-core/kern/partition.c b/grub-core/kern/partition.c
> index e499147..f27a8bd 100644
> --- a/grub-core/kern/partition.c
> +++ b/grub-core/kern/partition.c
> @@ -274,3 +274,26 @@ grub_partition_get_name (const grub_partition_t partition)
>    grub_memmove (out, ptr + 1, out + needlen - ptr);
>    return out;
>  }
> +
> +int grub_partition_is_type (struct grub_disk *disk,
> +			    const grub_partition_t partition,
> +			    const char *partmap_name,
> +			    const char *partition_type)
> +{
> +  char *type;
> +  int r;
> +
> +  if (!disk || !partition || !partition->partmap->type)
> +    return 0;
> +
> +  if (grub_strcmp (partition->partmap->name, partmap_name) != 0)
> +    return 0;
> +
> +  if (partition->partmap->type(disk, partition, &type) != 0)
> +    return 0;
> +
> +  r = grub_strcmp (type, partition_type);
> +  grub_free (type);
> +
> +  return r == 0;
> +}
> diff --git a/grub-core/partmap/gpt.c b/grub-core/partmap/gpt.c
> index 38df7b3..6d6c13f 100644
> --- a/grub-core/partmap/gpt.c
> +++ b/grub-core/partmap/gpt.c
> @@ -33,11 +33,10 @@ static grub_uint8_t grub_gpt_magic[8] =
>      0x45, 0x46, 0x49, 0x20, 0x50, 0x41, 0x52, 0x54
>    };
>  
> -static const grub_gpt_part_type_t grub_gpt_partition_type_empty = GRUB_GPT_PARTITION_TYPE_EMPTY;
> -
> -#ifdef GRUB_UTIL
> -static const grub_gpt_part_type_t grub_gpt_partition_type_bios_boot = GRUB_GPT_PARTITION_TYPE_BIOS_BOOT;
> -#endif
> +static const grub_gpt_part_type_t grub_gpt_partition_type_empty =
> +  { 0x0, 0x0, 0x0,
> +    { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 }
> +  };
>  
>  /* 512 << 7 = 65536 byte sectors.  */
>  #define MAX_SECTOR_LOG 7
> @@ -128,6 +127,40 @@ grub_gpt_partition_map_iterate (grub_disk_t disk,
>    return GRUB_ERR_NONE;
>  }
>  
> +static grub_err_t
> +grub_gpt_partition_type (grub_disk_t disk, const grub_partition_t partition,
> +			 char **type)
> +{
> +  struct grub_gpt_partentry gptentry;
> +  grub_gpt_part_type_t gpttype;
> +  grub_partition_t p2;
> +  grub_err_t err;
> +
> +  p2 = disk->partition;
> +  disk->partition = partition->parent;
> +  err = grub_disk_read (disk, partition->offset, partition->index,
> +			sizeof (gptentry), &gptentry);
> +  disk->partition = p2;
> +
> +  if (err)
> +    return err;
> +
> +  gpttype.data1 = grub_le_to_cpu32 (gptentry.type.data1);
> +  gpttype.data2 = grub_le_to_cpu16 (gptentry.type.data2);
> +  gpttype.data3 = grub_le_to_cpu16 (gptentry.type.data3);
> +  grub_memcpy (gpttype.data4, gptentry.type.data4, sizeof (gpttype.data4));
> +
> +  *type = grub_xasprintf ("%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> +			  gpttype.data1, gpttype.data2,
> +			  gpttype.data3, gpttype.data4[0],
> +			  gpttype.data4[1], gpttype.data4[2],
> +			  gpttype.data4[3], gpttype.data4[4],
> +			  gpttype.data4[5], gpttype.data4[6],
> +			  gpttype.data4[7]);
> +
> +  return GRUB_ERR_NONE;
> +}
> +
>  #ifdef GRUB_UTIL
>  /* Context for gpt_partition_map_embed.  */
>  struct gpt_partition_map_embed_ctx
> @@ -137,25 +170,14 @@ struct gpt_partition_map_embed_ctx
>  
>  /* Helper for gpt_partition_map_embed.  */
>  static int
> -find_usable_region (grub_disk_t disk __attribute__ ((unused)),
> +find_usable_region (grub_disk_t disk,
>  		    const grub_partition_t p, void *data)
>  {
>    struct gpt_partition_map_embed_ctx *ctx = data;
> -  struct grub_gpt_partentry gptdata;
> -  grub_partition_t p2;
> -
> -  p2 = disk->partition;
> -  disk->partition = p->parent;
> -  if (grub_disk_read (disk, p->offset, p->index,
> -		      sizeof (gptdata), &gptdata))
> -    {
> -      disk->partition = p2;
> -      return 0;
> -    }
> -  disk->partition = p2;
>  
>    /* If there's an embed region, it is in a dedicated partition.  */
> -  if (! grub_memcmp (&gptdata.type, &grub_gpt_partition_type_bios_boot, 16))
> +  if (grub_partition_is_type(disk, p, "gpt",
> +			     GRUB_GPT_PARTITION_TYPE_BIOS_BOOT))
>      {
>        ctx->start = p->start;
>        ctx->len = p->len;
> @@ -215,6 +237,7 @@ static struct grub_partition_map grub_gpt_partition_map =
>    {
>      .name = "gpt",
>      .iterate = grub_gpt_partition_map_iterate,
> +    .type = grub_gpt_partition_type,
>  #ifdef GRUB_UTIL
>      .embed = gpt_partition_map_embed
>  #endif
> diff --git a/grub-core/partmap/msdos.c b/grub-core/partmap/msdos.c
> index 1d81a53..68140a7 100644
> --- a/grub-core/partmap/msdos.c
> +++ b/grub-core/partmap/msdos.c
> @@ -228,6 +228,14 @@ grub_partition_msdos_iterate (grub_disk_t disk,
>    return grub_errno;
>  }
>  
> +static grub_err_t
> +grub_partition_msdos_type (grub_disk_t disk __attribute__ ((unused)),
> +			   const grub_partition_t partition, char **type)
> +{
> +  *type = grub_xasprintf ("%02x", partition->msdostype);
> +  return GRUB_ERR_NONE;
> +}
> +
>  #ifdef GRUB_UTIL
>  
>  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> @@ -415,6 +423,7 @@ static struct grub_partition_map grub_msdos_partition_map =
>    {
>      .name = "msdos",
>      .iterate = grub_partition_msdos_iterate,
> +    .type = grub_partition_msdos_type,
>  #ifdef GRUB_UTIL
>      .embed = pc_partition_map_embed
>  #endif
> diff --git a/include/grub/gpt_partition.h b/include/grub/gpt_partition.h
> index 1b32f67..566334c 100644
> --- a/include/grub/gpt_partition.h
> +++ b/include/grub/gpt_partition.h
> @@ -31,24 +31,12 @@ struct grub_gpt_part_type
>  } __attribute__ ((aligned(8)));
>  typedef struct grub_gpt_part_type grub_gpt_part_type_t;
>  
> -#define GRUB_GPT_PARTITION_TYPE_EMPTY \
> -  { 0x0, 0x0, 0x0, \
> -    { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } \
> -  }
> -
>  #define GRUB_GPT_PARTITION_TYPE_BIOS_BOOT \
> -  { grub_cpu_to_le32_compile_time (0x21686148), \
> -      grub_cpu_to_le16_compile_time (0x6449), \
> -      grub_cpu_to_le16_compile_time (0x6e6f),	       \
> -    { 0x74, 0x4e, 0x65, 0x65, 0x64, 0x45, 0x46, 0x49 } \
> -  }
> -
> +  "21686148-6449-6e6f-744e-656564454649"
>  #define GRUB_GPT_PARTITION_TYPE_LDM \
> -  { grub_cpu_to_le32_compile_time (0x5808C8AAU),\
> -      grub_cpu_to_le16_compile_time (0x7E8F), \
> -      grub_cpu_to_le16_compile_time (0x42E0),	       \
> -	{ 0x85, 0xD2, 0xE1, 0xE9, 0x04, 0x34, 0xCF, 0xB3 }	\
> -  }
> +  "5808c8aa-7e8f-42e0-85d2-e1e90434cfb3"
> +#define GRUB_GPT_PARTITION_TYPE_PREP \
> +  "9e1a2d38-c612-4316-aa26-8b49521e5a8b"
>  
>  struct grub_gpt_header
>  {
> diff --git a/include/grub/partition.h b/include/grub/partition.h
> index 7adb7ec..3dc46bf 100644
> --- a/include/grub/partition.h
> +++ b/include/grub/partition.h
> @@ -50,6 +50,13 @@ struct grub_partition_map
>    /* Call HOOK with each partition, until HOOK returns non-zero.  */
>    grub_err_t (*iterate) (struct grub_disk *disk,
>  			 grub_partition_iterate_hook_t hook, void *hook_data);
> +
> +  /* Return the type of the partition PARTITION in TYPE.
> +     The type is returned in a grub_malloc'ed buffer and should
> +     be freed by the caller.  */
> +  grub_err_t (*type) (struct grub_disk *disk,
> +		      const grub_partition_t partition, char **type);
> +
>  #ifdef GRUB_UTIL
>    /* Determine sectors available for embedding.  */
>    grub_err_t (*embed) (struct grub_disk *disk, unsigned int *nsectors,
> @@ -95,6 +102,10 @@ int EXPORT_FUNC(grub_partition_iterate) (struct grub_disk *disk,
>  					 grub_partition_iterate_hook_t hook,
>  					 void *hook_data);
>  char *EXPORT_FUNC(grub_partition_get_name) (const grub_partition_t partition);
> +int EXPORT_FUNC(grub_partition_is_type) (struct grub_disk *disk,
> +					 const grub_partition_t partition,
> +					 const char *partmap_name,
> +					 const char *partition_type);
>  
>  
>  extern grub_partition_map_t EXPORT_VAR(grub_partition_map_list);
> diff --git a/util/grub-install.c b/util/grub-install.c
> index 7d61c32..ee98b70 100644
> --- a/util/grub-install.c
> +++ b/util/grub-install.c
> @@ -696,36 +696,12 @@ write_to_disk (grub_device_t dev, const char *fn)
>  static int
>  is_prep_partition (grub_device_t dev)
>  {
> -  if (!dev->disk)
> -    return 0;
> -  if (!dev->disk->partition)
> -    return 0;
> -  if (strcmp(dev->disk->partition->partmap->name, "msdos") == 0)
> -    return (dev->disk->partition->msdostype == 0x41);
> -
> -  if (strcmp (dev->disk->partition->partmap->name, "gpt") == 0)
> -    {
> -      struct grub_gpt_partentry gptdata;
> -      grub_partition_t p = dev->disk->partition;
> -      int ret = 0;
> -      dev->disk->partition = dev->disk->partition->parent;
> +  if (grub_partition_is_type (dev->disk, dev->disk->partition, "msdos", "41"))
> +    return 1;
>  
> -      if (grub_disk_read (dev->disk, p->offset, p->index,
> -			  sizeof (gptdata), &gptdata) == 0)
> -	{
> -	  const grub_gpt_part_type_t template = {
> -	    grub_cpu_to_le32_compile_time (0x9e1a2d38),
> -	    grub_cpu_to_le16_compile_time (0xc612),
> -	    grub_cpu_to_le16_compile_time (0x4316),
> -	    { 0xaa, 0x26, 0x8b, 0x49, 0x52, 0x1e, 0x5a, 0x8b }
> -	  };
> -
> -	  ret = grub_memcmp (&template, &gptdata.type,
> -			     sizeof (template)) == 0;
> -	}
> -      dev->disk->partition = p;
> -      return ret;
> -    }
> +  if (grub_partition_is_type (dev->disk, dev->disk->partition,
> +			      "gpt", GRUB_GPT_PARTITION_TYPE_PREP))
> +    return 1;
>  
>    return 0;
>  }
> diff --git a/util/grub-probe.c b/util/grub-probe.c
> index ecb7b6b..a2505f0 100644
> --- a/util/grub-probe.c
> +++ b/util/grub-probe.c
> @@ -27,7 +27,6 @@
>  #include <grub/fs.h>
>  #include <grub/partition.h>
>  #include <grub/msdos_partition.h>
> -#include <grub/gpt_partition.h>
>  #include <grub/emu/hostdisk.h>
>  #include <grub/emu/getroot.h>
>  #include <grub/term.h>
> @@ -253,6 +252,25 @@ probe_abstraction (grub_disk_t disk, char delim)
>  }
>  
>  static void
> +probe_partition_type(grub_disk_t disk, const char *partmap, char delim)
> +{
> +  char *type;
> +
> +  if (!disk->partition || !disk->partition->partmap->type ||
> +      strcmp(disk->partition->partmap->name, partmap) != 0)
> +    {
> +      putchar (delim);
> +      return;
> +    }
> +
> +  if (disk->partition->partmap->type(disk, disk->partition, &type))
> +    grub_util_error ("%s", grub_errmsg);
> +
> +  printf ("%s%c", type, delim);
> +  free(type);
> +}
> +
> +static void
>  probe (const char *path, char **device_names, char delim)
>  {
>    char **drives_names = NULL;
> @@ -653,44 +671,14 @@ probe (const char *path, char **device_names, char delim)
>  
>        if (print == PRINT_MSDOS_PARTTYPE)
>  	{
> -	  if (dev->disk->partition
> -	      && strcmp(dev->disk->partition->partmap->name, "msdos") == 0)
> -	    printf ("%02x", dev->disk->partition->msdostype);
> -
> -	  putchar (delim);
> +	  probe_partition_type(dev->disk, "msdos", delim);
>  	  grub_device_close (dev);
>  	  continue;
>  	}
>  
>        if (print == PRINT_GPT_PARTTYPE)
>  	{
> -          if (dev->disk->partition
> -	      && strcmp (dev->disk->partition->partmap->name, "gpt") == 0)
> -            {
> -              struct grub_gpt_partentry gptdata;
> -              grub_partition_t p = dev->disk->partition;
> -              dev->disk->partition = dev->disk->partition->parent;
> -
> -              if (grub_disk_read (dev->disk, p->offset, p->index,
> -                                  sizeof (gptdata), &gptdata) == 0)
> -                {
> -                  grub_gpt_part_type_t gpttype;
> -                  gpttype.data1 = grub_le_to_cpu32 (gptdata.type.data1);
> -                  gpttype.data2 = grub_le_to_cpu16 (gptdata.type.data2);
> -                  gpttype.data3 = grub_le_to_cpu16 (gptdata.type.data3);
> -                  grub_memcpy (gpttype.data4, gptdata.type.data4, 8);
> -
> -                  grub_printf ("%08x-%04x-%04x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> -                               gpttype.data1, gpttype.data2,
> -                               gpttype.data3, gpttype.data4[0], 
> -                               gpttype.data4[1], gpttype.data4[2],
> -                               gpttype.data4[3], gpttype.data4[4],
> -                               gpttype.data4[5], gpttype.data4[6],
> -                               gpttype.data4[7]);
> -                }
> -              dev->disk->partition = p;
> -            }
> -          putchar (delim);
> +	  probe_partition_type(dev->disk, "gpt", delim);
>            grub_device_close (dev);
>            continue;
>          }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Vladimir 'φ-coder/phcoder' Serbinenko]

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

On 22.09.2014 22:03, Michael Marineau wrote:
> that is what I need to make use of and copying the code to re-read the
> type field yet again felt silly. My final goal is actually a module
> that will search for two partitions of the same type and select
> between them based on some other metadata. Since searching by
> partition type is already a common task in the grub code base
> providing an API to do so seemed like a good starting point. Others
> will likely find extending this to support label and uuid more useful.
> For example with GPT the Linux kernel can use
> "root=PARTUUID=<someuuid>" directly, unlike LABEL= or UUID= which must
> be implemented in an initrd.
Using partition type has proven to be unrelaiable in the past and grub2
uses as little of it as possible. Why can't your goals be achieved with
FS UUIDs?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Michael Marineau]

On Mon, Sep 22, 2014 at 4:06 PM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 22.09.2014 22:03, Michael Marineau wrote:
>> that is what I need to make use of and copying the code to re-read the
>> type field yet again felt silly. My final goal is actually a module
>> that will search for two partitions of the same type and select
>> between them based on some other metadata. Since searching by
>> partition type is already a common task in the grub code base
>> providing an API to do so seemed like a good starting point. Others
>> will likely find extending this to support label and uuid more useful.
>> For example with GPT the Linux kernel can use
>> "root=PARTUUID=<someuuid>" directly, unlike LABEL= or UUID= which must
>> be implemented in an initrd.
> Using partition type has proven to be unrelaiable in the past and grub2
> uses as little of it as possible. Why can't your goals be achieved with
> FS UUIDs?

I'm working on CoreOS which updates /usr via read-only filesystem
images, switching between using two different partitions in an A/B
setup. The UUID in the filesystem is set when the image is created and
has nothing to do with which of the two coppies of the filesystem I
want to boot. That information is instead in the partition table, both
/usr partitions have the same special type and a set of flags that
determine which of the two to try next. It is based on the scheme
ChromeOS uses except we only have /usr instead of root and kernel
partitions.

http://www.chromium.org/chromium-os/chromiumos-design-docs/disk-format#TOC-Selecting-the-kernel

Right now we implement this logic in an initrd and launch the final
kernel (which must match the /usr partition) via kexec. Instead I'm
moving that into a grub module because kexec does not work on many
platforms.

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Michael Marineau]

On Mon, Sep 22, 2014 at 4:04 PM, Vladimir 'φ-coder/phcoder' Serbinenko
<phcoder@gmail.com> wrote:
> On 22.09.2014 20:59, Michael Marineau wrote:
>> This introduces a new type() function for partmaps modeled after the
>> label() and uuid() functions for filesystems. A likely future extension
>> will be support partition labels and uuids as well. This is in
>> preparation for adding more functionality like a search.part_type
>> command.
>>
>> The msdos partition code is only partially converted for now, I'm not
>> sure about the preferred way to do this. Re-reading a msdos partition
>> table is not as easy as re-reading gpt is. One option would be to turn
>> 'msdostype' into a union or data pointer that partmap modules can stash
>> extra info in. That would also allow gpt to avoid re-reading. Being new
>> to grub, are there memory usage concerns I should be aware of here?
> Please provide a usecase. Every new feature needs a usecase. This patch
> increases core size and will not go in without a damn good usecase and
> justification why it needs to be done in core.

In reality I don't need to put anything into core, everything I need
can happily live in a brand new self-contained module. Since I need to
look up partitions by type it looked like a good opportunity to
consolidate the existing duplicated code for looking up GPT types into
a common API. If you have an alternate suggestion for cleaning up that
duplication I'm all ears, that's why I posted this early patch for
comments. :)

Re: [RFC PATCH] Add new partition type API and convert GPT type checks.

[Andrei Borzenkov]

В Mon, 22 Sep 2014 16:49:46 -0700
Michael Marineau <michael.marineau@coreos.com> пишет:

> On Mon, Sep 22, 2014 at 4:06 PM, Vladimir 'φ-coder/phcoder' Serbinenko
> <phcoder@gmail.com> wrote:
> > On 22.09.2014 22:03, Michael Marineau wrote:
> >> that is what I need to make use of and copying the code to re-read the
> >> type field yet again felt silly. My final goal is actually a module
> >> that will search for two partitions of the same type and select
> >> between them based on some other metadata. Since searching by
> >> partition type is already a common task in the grub code base
> >> providing an API to do so seemed like a good starting point. Others
> >> will likely find extending this to support label and uuid more useful.
> >> For example with GPT the Linux kernel can use
> >> "root=PARTUUID=<someuuid>" directly, unlike LABEL= or UUID= which must
> >> be implemented in an initrd.
> > Using partition type has proven to be unrelaiable in the past and grub2
> > uses as little of it as possible. Why can't your goals be achieved with
> > FS UUIDs?
> 
> I'm working on CoreOS which updates /usr via read-only filesystem
> images, switching between using two different partitions in an A/B
> setup. The UUID in the filesystem is set when the image is created and
> has nothing to do with which of the two coppies of the filesystem I
> want to boot. That information is instead in the partition table, both
> /usr partitions have the same special type and a set of flags that
> determine which of the two to try next. It is based on the scheme
> ChromeOS uses except we only have /usr instead of root and kernel
> partitions.
> 
> http://www.chromium.org/chromium-os/chromiumos-design-docs/disk-format#TOC-Selecting-the-kernel
> 
> Right now we implement this logic in an initrd and launch the final
> kernel (which must match the /usr partition) via kexec. Instead I'm
> moving that into a grub module because kexec does not work on many
> platforms.
> 

It has GPT as prerequisite, and does not look like it would work with
anything else. Just implement support in loader module. If more users
appear, it is always possible to factor out common code.