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

[Glenn Washburn]

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