Re: [PATCH v7 05/17] luks2: Add json_slot_key member to struct grub_luks2_keyslot/segment/digest

[Glenn Washburn]

On Tue, 8 Dec 2020 07:38:31 +0100
Patrick Steinhardt <ps@pks.im> wrote:

> On Mon, Dec 07, 2020 at 10:06:44PM -0600, Glenn Washburn wrote:
> > On Mon, 7 Dec 2020 21:02:39 +0100
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> > 
> > > On Sun, Dec 06, 2020 at 02:29:06PM +0100, Patrick Steinhardt
> > > wrote:
> > > > On Fri, Dec 04, 2020 at 10:43:34AM -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 for both keyslot and segment slot keys.
> > > > >
> > > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > >
> > > > Personally, I'd have named them `json_slot_idx`. But you've
> > > > already done so much work on improving the code that I don't
> > > > want this to be the reason to not give an SOB, especially
> > > > considering that it's a strict improvement anyway. So:
> > > >
> > > > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > > 
> > > I can change this to json_slot_idx before committing if Glenn
> > > does not object. Otherwise Reviewed-by: Daniel Kiper
> > > <daniel.kiper@oracle.com>
> > 
> > Thanks Patrick for the sentiment.  Looking at the luks2 spec, it
> > says:
> > 
> > "JSON  objects  must  have  their names  formatted  as  a  string
> > that represents  a  number  in  the  decimal  notation (unsigned
> > integer) – for example ”0”, ”1” and must contain attribute _type_.
> > According  to the _type_,  the  implementation  decides  how  to
> > handle  (or ignore) such an object.  This notation allows mapping
> > to LUKS1 API functions that use an integer as a reference to
> > keyslots objects."
> > 
> > So here, the spec is calling that value a "name", and saying that it
> > must be a string of decimal digits.  Looking at the spec, the
> > "name" of the keyslot object does not need to correspond to the
> > index into the array of keyslot areas on disk.  However it does in
> > the canonical implementation for use with LUKS1 API functions which
> > require and integer, as suggested in the quote above.
> > 
> > I'd say that these numbers are actually an id for the object of its
> > respective class.  In the cryptsetup implementation, the "id"
> > happens to correspond to the index into the binary key area array,
> > but that's needn't be the case. My preference would be to name it
> > "id" and second choice would be just "idx" (since its usually an
> > index into a physical array of key areas or virtual array of
> > segments and digests).
> > 
> > I don't think the "json" part of the name is warranted, because it
> > really has nothing to do with json.  The "slot" part is really only
> > applicable to keyslots because digests and segments don't have an
> > equivalent slot aspect.  So I suggest we name the struct member
> > names to just "id" instead.  And where we just the index of the
> > name-value pair in the json associative array we use "json_idx" as
> > a suffix.  So this would mean changing the argument keyslot_idx in
> > luks2_get_keyslot() to keyslot_json_idx. Optionally the local
> > variable "i" in luks2_get_keyslot() and luks2_parse_segment() could
> > be renamed to "json_idx" as well (I don't care either way about
> > this though).
> > 
> > Glenn
> 
> Sounds sensible to me. Based on your reasoning, I'm happy with either
> "id" or "key". So we may just as well just keep it as-is, as you
> prefer.
> 
> Patrick

On third thought, while I like "id" better because its really a generic
id, we use it as an index into the segments and keyslots bitfield of
the grub_luks2_digest_t struct. So I'm going to I'm going to choose idx
instead of id.  If the implementation changes and we choose to support
arbitrary object names we can change the member name to something more
generic.

Glenn