From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kkrkU-0007Ou-EO for mharc-grub-devel@gnu.org; Thu, 03 Dec 2020 11:46:30 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:44990) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kkrkS-0007OF-JK for grub-devel@gnu.org; Thu, 03 Dec 2020 11:46:28 -0500 Received: from mail-qt1-x842.google.com ([2607:f8b0:4864:20::842]:44689) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kkrkP-0006em-7q for grub-devel@gnu.org; Thu, 03 Dec 2020 11:46:28 -0500 Received: by mail-qt1-x842.google.com with SMTP id u21so1774518qtw.11 for ; Thu, 03 Dec 2020 08:46:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=LwkJgkwMEWZegtXuGe7NjnHJrILaX1ijqvx4loBusOc=; b=y1baVzDBH9C9KlV/GSpcJ+AFI6WFVYvHS6LtnnlWQzLobv2K4+A2n/ZZoQ5Z48s8Nj ptof/4uHj5u4sHE3lxcbnl3f+abOZ9ukpP5KQ7N/oeEi7jXdFolMOko/SKSNIGYjooQQ 7pKq8NAD8g1ebl0cfrUyA5WfVEkprkWpp6rtGVKCwD4jJpJ+c7aWiA0rTGApoNcWOMsD +5vSgezc4a0AC9MQi9JLJ7QvvXMQkzmbZkrg+fd4LPdwD4JhcwQBP7u2+5nk1/o9Vr5a 7g1Li3//Qy0r7jsfM4qeopyVNNcaf/2yh/UjueN2jgQfIuvE22oCOo7gG7RYdpT0Wflu n65A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=LwkJgkwMEWZegtXuGe7NjnHJrILaX1ijqvx4loBusOc=; b=S+l5L8lQgT5UcIjxnumh65+I/p13xNw0IBCuiZ5wAUN97NlVAlTwJmPd1ZFXWiCNdP 5t+KXI/88x2kjE1+syExX9mANOwqiVPWzSptu7C1ptyf/s+B+FPBLAfxk97N/YvhB3S0 8yL/GlG1LzWZwCZj1DSshu7c+MuRIxuqlr9d7nzXzVzJgYKs+CS8DQZHPXxPTgpFwLJJ XgoY+FfKt1C8ebTiaER/lv5Sz1CRar6UskpDFpHzcWMWS62STJMmz0EFtzubiqt8zJE2 ao4ndomIcSDXRBnPkLsBxxi2JA3UcicFNoQmOEk33+xFjQNWhPv1YEj2pegk35trEFM6 ojeQ== X-Gm-Message-State: AOAM533b0KaWnlKqy+zToDg5G9buqai6jYz0ctrgqqmHsF2LKhf4DHZd WcoiWN8tRVJPkwBDXCeZvf8g4kIiLcwBig== X-Google-Smtp-Source: ABdhPJyPtvKijnFi/7kFwd61mz6H/NPak33qQ5rHoTrlfvPgV2pMEIo6rN/sPShyrBGG+J2T2W6oVA== X-Received: by 2002:ac8:6c2d:: with SMTP id k13mr3156445qtu.197.1607013983728; Thu, 03 Dec 2020 08:46:23 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([136.49.211.192]) by smtp.gmail.com with ESMTPSA id t133sm1904989qke.82.2020.12.03.08.46.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 03 Dec 2020 08:46:23 -0800 (PST) Date: Thu, 3 Dec 2020 10:46:12 -0600 From: Glenn Washburn To: Daniel Kiper Cc: grub-devel@gnu.org, Patrick Steinhardt Subject: Re: [PATCH v6 01/12] luks2: Add slot_key member to struct grub_luks2_keyslot/segment/digest Message-ID: <20201203104612.32d58252@crass-HP-ZBook-15-G2> In-Reply-To: <20201203123528.dwvwzhypdoxfk42n@tomti.i.net-space.pl> References: <6262aefe9f1fe7e21d1c34518a70cbd3cc15573b.1606467382.git.development@efficientek.com> <20201202170147.sedm2ndxujtqrf2e@tomti.i.net-space.pl> <20201203012317.01e21775@crass-HP-ZBook-15-G2> <20201203123528.dwvwzhypdoxfk42n@tomti.i.net-space.pl> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::842; envelope-from=development@efficientek.com; helo=mail-qt1-x842.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Dec 2020 16:46:28 -0000 On Thu, 3 Dec 2020 13:35:28 +0100 Daniel Kiper wrote: > On Thu, Dec 03, 2020 at 01:23:17AM -0600, Glenn Washburn wrote: > > On Wed, 2 Dec 2020 18:01:47 +0100 > > Daniel Kiper 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 > > > > --- > > > > 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? > > IMO "key" is more confusing in this context and it would require at > least a comment to clarify what it means. Is not json_slot_key better > then? Probably with some comment what is it too... Yeah, that's a better name in terms of its descriptive power, but feels kinda long. I'm fine with using that though, and will use that for the next patch series, unless I hear otherwise. > > > > 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) || > > Should not keyslot be renamed to json_slot_key too? If I understand you correctly, you're asking about the variable keyslot of type grub_json_t. Here keyslot is a json object representing a keyslot (ie collection of parameters used to decrypt an encrypted key). It is not what I call a "slot key", which is the value of a key in a json associative array. So keyslot is a json object, where as a slot key is always a json string (afaik). I believe that it is currently appropriately named. Glenn