Re: [PATCH v6 01/12] luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest

[Glenn Washburn]

On Wed, 2 Dec 2020 18:01:47 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Nov 27, 2020 at 03:03:33AM -0600, Glenn Washburn wrote:
> > This allows code using these structs to know the named key
> > associated with these json data structures. In the future we can
> > use these to provide better error messages to the user.
> >
> > Get rid of idx variable in luks2_get_keyslot which was overloaded
> > to be used
> 
> I prefer if you add "()" to the function names, i.e.
> luks2_get_keyslot(), in the comments and commit messages. This way it
> is easier to understand what you mean.

Ok.

> > for both keyslot and segment slot keys.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/luks2.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index d96764a02..ab2c31dcd 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -65,6 +65,7 @@ typedef struct grub_luks2_header
> > grub_luks2_header_t;
> >
> >  struct grub_luks2_keyslot
> >  {
> > +  grub_uint64_t slot_key;
> 
> Could you be more consistent and use keyslot instead of slot_key here?

I intentionally chose not to use keyslot because I thought it was
confusing.  slot_key is not a keyslot in the sense of a slot for a
cryptographic key or key material as in the usage keyslot in the struct
named "grub_luks2_keyslot".  Its the key value of a "slot" in a json
associative array which is modeling a sparse array.  Perhaps just "key"
might be more to your liking?

> >    grub_int64_t key_size;
> >    grub_int64_t priority;
> >    struct
> > @@ -103,6 +104,7 @@ typedef struct grub_luks2_keyslot
> > grub_luks2_keyslot_t;
> >
> >  struct grub_luks2_segment
> >  {
> > +  grub_uint64_t slot_key;
> 
> Ditto. The code below uses keyslot instead...
> 
> >    grub_uint64_t offset;
> >    const char       *size;
> >    const char       *encryption;
> > @@ -112,6 +114,7 @@ typedef struct grub_luks2_segment
> > grub_luks2_segment_t;
> >
> >  struct grub_luks2_digest
> >  {
> > +  grub_uint64_t slot_key;
> >    /* Both keyslots and segments are interpreted as bitfields here
> > */ grub_uint64_t    keyslots;
> >    grub_uint64_t    segments;
> > @@ -259,12 +262,11 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > grub_luks2_digest_t *d, grub_luks2_s {
> >    grub_json_t keyslots, keyslot, digests, digest, segments,
> > segment; grub_size_t i, size;
> > -  grub_uint64_t idx;
> >
> >    /* Get nth keyslot */
> >    if (grub_json_getvalue (&keyslots, root, "keyslots") ||
> >        grub_json_getchild (&keyslot, &keyslots, keyslot_idx) ||
> > -      grub_json_getuint64 (&idx, &keyslot, NULL) ||
> > +      grub_json_getuint64 (&k->slot_key, &keyslot, NULL) ||
> 
> Daniel