Re: [PATCH 1/1] add --partuuid to probe

[Vladimir 'phcoder' Serbinenko]

On Wed, Feb 15, 2017, 17:27 Andrei Borzenkov <address@hidden> wrote:

15.02.2017 13:56, Vladimir 'phcoder' Serbinenko пишет:
> On Tue, Feb 14, 2017, 19:01 Steve Kenton <address@hidden> wrote:
>
>> Support both EFI and NT Disk Signature for passing to kernel as
>> root=PARTUUID=$val
>>
>> Signed-off-by: Steve Kenton <address@hidden>
>> ---
>> It's been six months so I thought I'd resend this so it does not get lost
>> in case I get hit by a meteor or something before the next release
>>
>>  grub-core/commands/probe.c | 53
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 53 insertions(+)
>>
>> diff --git a/grub-core/commands/probe.c b/grub-core/commands/probe.c
>> index cf2793e..3afc8b8 100644
>> --- a/grub-core/commands/probe.c
>> +++ b/grub-core/commands/probe.c
>> @@ -45,6 +45,7 @@ static const struct grub_arg_option options[] =
>>      {"fs",             'f', 0, N_("Determine filesystem type."), 0, 0},
>>      {"fs-uuid",                'u', 0, N_("Determine filesystem UUID."),
>> 0, 0},
>>      {"label",          'l', 0, N_("Determine filesystem label."), 0, 0},
>> +    {"partuuid",       'g', 0, N_("Determine partition GUID/UUID."), 0,
>> 0}, /* GUID but Linux kernel calls it "PARTUUID" */
>>
> I like how it generalizes.
>
>>      {0, 0, 0, 0, 0, 0}
>>    };
>>
>> @@ -154,6 +155,58 @@ grub_cmd_probe (grub_extcmd_context_t ctxt, int argc,
>> char **args)
>>        grub_device_close (dev);
>>        return GRUB_ERR_NONE;
>>      }
>> +  if (state[6].set)
>> +    {
>> +      char *partuuid = NULL; /* NULL to silence a spurious GCC warning */
>> +      grub_uint8_t diskbuf[16];
>> +      if (dev->disk && dev->disk->partition)
>> +       {
>> +         grub_partition_t p = dev->disk->partition;
>> +         if (!grub_strcmp (p->partmap->name, "msdos"))
>>
> Please use == 0 rather than !
>
>> +           {
>> +             const int diskid_offset = 440; /* location in MBR */
>>
> Please get this from a common header rather than hard coding. I think we
> have it in msdos.h
>
>> +             dev->disk->partition = p->parent;
>> +             /* little-endian 4-byte NT disk signature */
>> +             err = grub_disk_read (dev->disk, 0, diskid_offset, 4,
>> diskbuf);
>> +             dev->disk->partition = p;
>> +             if (err)
>> +               return grub_errno;
>> +             partuuid = grub_xasprintf ("%02x%02x%02x%02x-%02x",
>> +                                        diskbuf[3], diskbuf[2],
>> diskbuf[1], diskbuf[0],
>> +                                        p->number + 1); /* one based
>> partition number */
>>
> This is not NT-style. NT uses partition offset. Who uses this format? Are

This is used by util-linux and Linux kernel.


 *      6) PARTUUID=00112233-4455-6677-8899-AABBCCDDEEFF representing the
 *         unique id of a partition if the partition table provides it.
 *         The UUID may be either an EFI/GPT UUID, or refer to an MSDOS
 *         partition using the format SSSSSSSS-PP, where SSSSSSSS is a zero-
 *         filled hex representation of the 32-bit "NT disk signature",
and PP
 *         is a zero-filled hex representation of the 1-based partition
number.

> you sure that partition numbers are synced with user? Even in presence of
> Solaris and bsd partitions.
>

It is not clear what we should return for nested partition. I'm not sure
whether linux kernel scans nested partitions at all in which case we
probably should follow the suite and assign PARTUUID to top-level
partitions only.

Linux scans nested partitions and it uses though numeration in dev/sdaX, in some cases shifting numbering of normal partitions. In those cases grub and Linux numeration get out of sync

>> +           }
>> +         else if (!grub_strcmp (p->partmap->name, "gpt"))
>>
> Ditto.
>
>> +           {
>> +             const int guid_offset = 16; /* location in entry */
>>
> Ditto.
>
>> +             dev->disk->partition = p->parent;
>> +             /* little-endian 16-byte EFI partition GUID */
>> +             err = grub_disk_read (dev->disk, p->offset, p->index +
>> guid_offset, 16, diskbuf);
>> +             dev->disk->partition = p;
>> +             if (err)
>> +               return grub_errno;
>> +             partuuid = grub_xasprintf
>> ("%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
>> +                                        diskbuf[3], diskbuf[2],
>> diskbuf[1], diskbuf[0],
>> +                                        diskbuf[5], diskbuf[4],
>> +                                        diskbuf[7], diskbuf[6],
>> +                                        diskbuf[8], diskbuf[9],
>> +                                        diskbuf[10], diskbuf[11],
>> diskbuf[12], diskbuf[13], diskbuf[14], diskbuf[15]);
>> +           }
>> +         else
>> +           return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
>> +                              N_("partition map %s does not support
>> partition UUIDs"),
>> +                              dev->disk->partition->partmap->name);
>> +       }
>> +      else
>> +       partuuid = grub_strdup (""); /* a freeable empty string */
>> +
>> +      if (state[0].set)
>> +       grub_env_set (state[0].arg, partuuid);
>> +      else
>> +       grub_printf ("%s", partuuid);
>> +      grub_free (partuuid);
>> +      grub_device_close (dev);
>> +      return GRUB_ERR_NONE;
>> +    }
>>    grub_device_close (dev);
>>    return grub_error (GRUB_ERR_BAD_ARGUMENT, "unrecognised target");
>>  }
>> --
>> 2.9.0.137.gcf4c2cf
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> https://lists.gnu.org/mailman/listinfo/grub-devel
>>
>
>
>
> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel
>


_______________________________________________
Grub-devel mailing list
address@hidden
https://lists.gnu.org/mailman/listinfo/grub-devel