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

[Vladimir 'φ-coder/phcoder' Serbinenko]

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;
>          }
>

signature.asc
Description: OpenPGP digital signature