From: "Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>
To: The development of GNU GRUB <grub-devel@gnu.org>
Cc: Michael Marineau <michael.marineau@coreos.com>
Subject: Re: [RFC PATCH] Add new partition type API and convert GPT type checks.
Date: Tue, 23 Sep 2014 01:04:49 +0200 [thread overview]
Message-ID: <5420AB11.5080002@gmail.com> (raw)
In-Reply-To: <1411412346-4003-1-git-send-email-michael.marineau@coreos.com>
[-- 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 --]
next prev parent reply other threads:[~2014-09-22 23:05 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-22 18:59 [RFC PATCH] Add new partition type API and convert GPT type checks Michael Marineau
2014-09-22 19:19 ` Andrei Borzenkov
2014-09-22 20:03 ` Michael Marineau
2014-09-22 23:06 ` Vladimir 'φ-coder/phcoder' Serbinenko
2014-09-22 23:49 ` Michael Marineau
2014-09-23 17:27 ` Andrei Borzenkov
2014-09-22 23:04 ` Vladimir 'φ-coder/phcoder' Serbinenko [this message]
2014-09-23 0:25 ` Michael Marineau
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5420AB11.5080002@gmail.com \
--to=phcoder@gmail.com \
--cc=grub-devel@gnu.org \
--cc=michael.marineau@coreos.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.