Re: [GRUB PARTUUID PATCH V7 3/4] Add PARTUUID detection support to grub-probe

[Daniel Kiper]

On Tue, Mar 27, 2018 at 09:30:10PM -0700, Nick Vinson wrote:
> On 03/27/2018 01:52 PM, Daniel Kiper wrote:
> > On Mon, Mar 26, 2018 at 11:07:58PM -0700, Nicholas Vinson wrote:
> >> Add PARTUUID detection support grub-probe for MBR and GPT partition
> >> schemes.
> >>
> >> Signed-off-by: Nicholas Vinson <address@hidden>
> >> ---
> >>  util/grub-probe.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 48 insertions(+)
> >>
> >> diff --git a/util/grub-probe.c b/util/grub-probe.c
> >> index 21cb80fbe..48ef1e2ec 100644
> >> --- a/util/grub-probe.c
> >> +++ b/util/grub-probe.c
> >> @@ -28,6 +28,7 @@
> >>  #include <grub/partition.h>
> >>  #include <grub/msdos_partition.h>
> >>  #include <grub/gpt_partition.h>
> >> +#include <grub/i386/pc/boot.h>
> >>  #include <grub/emu/hostdisk.h>
> >>  #include <grub/emu/getroot.h>
> >>  #include <grub/term.h>
> >> @@ -62,6 +63,7 @@ enum {
> >>    PRINT_DRIVE,
> >>    PRINT_DEVICE,
> >>    PRINT_PARTMAP,
> >> +  PRINT_PARTUUID,
> >>    PRINT_ABSTRACTION,
> >>    PRINT_CRYPTODISK_UUID,
> >>    PRINT_HINT_STR,
> >> @@ -85,6 +87,7 @@ static const char *targets[] =
> >>      [PRINT_DRIVE]              = "drive",
> >>      [PRINT_DEVICE]             = "device",
> >>      [PRINT_PARTMAP]            = "partmap",
> >> +    [PRINT_PARTUUID]           = "partuuid",
> >>      [PRINT_ABSTRACTION]        = "abstraction",
> >>      [PRINT_CRYPTODISK_UUID]    = "cryptodisk_uuid",
> >>      [PRINT_HINT_STR]           = "hints_string",
> >> @@ -181,6 +184,45 @@ probe_partmap (grub_disk_t disk, char delim)
> >>      }
> >>  }
> >>
> >> +static void
> >> +probe_partuuid (grub_disk_t disk, char delim)
> >> +{
> >> +  grub_partition_t p = disk->partition;
> >
> > Lack of empty line.
>
> added an empty line.
>
> >
> >> +  /*
> >> +   * Nested partitions not supported for now.
> >> +   * Non-nested partitions must have disk->partition->parent == NULL
> >> +   */
> >> +  if (disk->partition && disk->partition->parent == NULL)
> >> +    {
> >> +      if (strcmp(disk->partition->partmap->name, "msdos") == 0)
> >> +  {
> >> +      /*
> >> +       * The partition GUID for MSDOS is the partition number (starting
> >> +       * with 1) prepended with the NT disk signature.
> >> +       */
> >> +      grub_uint32_t nt_disk_sig;
> >> +      disk->partition = p->parent;
> >> +
> >> +      if (grub_disk_read (disk, 0, GRUB_BOOT_MACHINE_WINDOWS_NT_MAGIC,
> >> +                          sizeof(nt_disk_sig), &nt_disk_sig) == 0)
> >> +
> >
> > Redundant empty line.
>
> removed the empty line.
> >
> >> +          grub_printf ("%08x-%02x",
> >> +                       grub_le_to_cpu32(nt_disk_sig), 1 + p->number);
> >> +  }
> >> +      else if (strcmp(disk->partition->partmap->name, "gpt") == 0)
> >> +  {
> >> +    struct grub_gpt_partentry gptdata;
> >> +
> >> +    disk->partition = p->parent;
> >> +
> >> +    if (grub_disk_read (disk, p->offset, p->index,
> >> +                        sizeof(gptdata), &gptdata) == 0)
> >> +      print_gpt_guid(gptdata.guid);
> >> +  }
> >
> > Why "disk->partition = p;" is not here?
> > Because compiler complains if it is here?
>
> I misread my own diff and thought you had asked for me to put it below
> instead of here.  Either location works, so it's not too critical where
> it's put.

Great! So, please put it there where I have asked for earlier.

> > Anyway, if I know the reason for above I can fix
> > earlier ntipicks myself before commiting this patch.>
> > Well, "disk->partition = p->parent;" can be before
> > "if (strcmp(disk->partition->partmap->name, "msdos") == 0)".
> > Am I right?
>
> Yes, but it'll require some changes to the checks to make such a change
> work.  For example, "disk->partition->partmap->name" would have to be
> changed to "p->partmap->name"

I am OK with that change.

> I'll send a second email with an updated patch for this commit.  If you
> would like me to regenerate the entire patch set, please let me know.

Entire patchset please. If you do not change anything in other patches
you can add my RB to them.

Daniel