grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v7 05/17] luks2: Add json_slot_key member to struct grub_luks


From: Patrick Steinhardt
Subject: Re: [PATCH v7 05/17] luks2: Add json_slot_key member to struct grub_luks2_keyslot/segment/digest
Date: Tue, 8 Dec 2020 07:38:31 +0100

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

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]