[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
signature.asc
Description: PGP signature
- Re: [PATCH v7 03/17] luks2: Remove unused argument in grub_error, (continued)
[PATCH v7 14/17] whitespace: convert 8 spaces to tabs, Glenn Washburn, 2020/12/04
Re: [PATCH v7 00/17] Cryptodisk fixes for v2.06 redux, Patrick Steinhardt, 2020/12/06