From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mXJa1-0004DA-4C for mharc-grub-devel@gnu.org; Mon, 04 Oct 2021 04:44:13 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54744) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXJZw-00048r-Tt for grub-devel@gnu.org; Mon, 04 Oct 2021 04:44:09 -0400 Received: from wout3-smtp.messagingengine.com ([64.147.123.19]:54771) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXJZu-0008Bt-Nm for grub-devel@gnu.org; Mon, 04 Oct 2021 04:44:08 -0400 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.west.internal (Postfix) with ESMTP id 31DA332007E8; Mon, 4 Oct 2021 04:44:04 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute6.internal (MEProxy); Mon, 04 Oct 2021 04:44:04 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pks.im; h=date :from:to:cc:subject:message-id:references:mime-version :content-type:in-reply-to; s=fm2; bh=CJcALim6Iqqs+bLAk9ghjtT3Z7b r4L4EaS6zfrs3IeI=; b=pyJvicNF5fuKaYN9H/3XPVHO1ZPH/xYcmGRhfUCk7rF 0HxEGg+5+7u8vV98MzJ4KdDuYBQzA95URzyfN7yPz5y7NJEr1l0UlSNT5MIntGFV YskxzYgxATJCoINr5jMoCWS/5zv1+BXlZ2aUZtpF4Ey4blA3BPC9aYiQWhFZhK3q VbIalQBGk6aUE///HQZLZ5zhByfumcn8GnfBunziwp8ajAnithMdBCaTo9xJsydd fMGyy7lxJATOFr6FRKdjZbOE07oRTgqYTAxZizQCJGuuaPSE+QCk0rqJBsIxy2NE wmuwBr6+0MEwjxxqtKXAPMYo1XOC5Is3ECPFrqctIJA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm1; bh=CJcALi m6Iqqs+bLAk9ghjtT3Z7br4L4EaS6zfrs3IeI=; b=D0A22SPT/UZH81nrBY0Nq9 gJswSzUzFRZsXV1IjKUliydFQiZUIgDyku96yzH4V3w7yepSyLgy50OIt7cuu6Kr KUWQMXUKIO/j0nnaCzFEtoWpsWsz1XhNYyDcSn3qfJKDRBD75oSj3HOVcg3r/HBN AFc7M3n6+dGf5A8bZm9tpNT0z3tSDPSU/E1QiED4MnSLuaGub8iXjOA7pW4MdsQ/ vZbZpZVPIV7Nj4eOnOKySk4RLTEyf+M3Al1ZCifbkAr1Pw5lbk3A752kVmqcHrsM vqyoGlMkovY7LLRyZz/zpinQCYWdeWQVUuFdyUnYZ01PEsziNz4RD9BrOhrvMTlg == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrudelvddgtdehucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhepfffhvffukfhfgggtuggjsehgtderredttddvnecuhfhrohhmpefrrghtrhhi tghkucfuthgvihhnhhgrrhguthcuoehpshesphhkshdrihhmqeenucggtffrrghtthgvrh hnpeehgefhtdefueffheekgfffudelffejtdfhvdejkedthfehvdelgfetgfdvtedthfen ucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehpshesph hkshdrihhm X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Mon, 4 Oct 2021 04:44:02 -0400 (EDT) Received: from localhost (ncase [10.192.0.11]) by vm-mail.pks.im (OpenSMTPD) with ESMTPSA id 4983460c (TLSv1.3:TLS_AES_256_GCM_SHA384:256:NO); Mon, 4 Oct 2021 08:43:58 +0000 (UTC) Date: Mon, 4 Oct 2021 10:43:50 +0200 From: Patrick Steinhardt To: Glenn Washburn Cc: grub-devel@gnu.org, Daniel Kiper , Denis 'GNUtoo' Carikli , James Bottomley Subject: Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct Message-ID: References: <20210927231403.642857-1-development@efficientek.com> <20210927231403.642857-4-development@efficientek.com> <20211003185745.454aa5f7@crass-HP-ZBook-15-G2> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="uhOx/5+z0BE9lHK7" Content-Disposition: inline In-Reply-To: <20211003185745.454aa5f7@crass-HP-ZBook-15-G2> Received-SPF: pass client-ip=64.147.123.19; envelope-from=ps@pks.im; helo=wout3-smtp.messagingengine.com X-Spam_score_int: -27 X-Spam_score: -2.8 X-Spam_bar: -- X-Spam_report: (-2.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_PASS=-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: Mon, 04 Oct 2021 08:44:10 -0000 --uhOx/5+z0BE9lHK7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Oct 03, 2021 at 06:57:45PM -0500, Glenn Washburn wrote: > On Sun, 3 Oct 2021 21:16:09 +0200 > Patrick Steinhardt wrote: >=20 > > On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote: > > > Signed-off-by: Glenn Washburn > > > --- > > > grub-core/disk/cryptodisk.c | 26 +++++++++----------------- > > > grub-core/disk/geli.c | 9 ++++----- > > > grub-core/disk/luks.c | 11 +++++------ > > > grub-core/disk/luks2.c | 6 +++--- > > > include/grub/cryptodisk.h | 6 ++++-- > > > 5 files changed, 25 insertions(+), 33 deletions(-) > > >=20 > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > > > index 86eaabe60..5e153ee0a 100644 > > > --- a/grub-core/disk/cryptodisk.c > > > +++ b/grub-core/disk/cryptodisk.c > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk) > > > =20 > > > #endif > > > =20 > > > -static int check_boot, have_it; > >=20 > > We should really just get rid of `have_it`. It's only used in three > > locations, and we only require it because > > `grub_cryptodisk_scan_device_real` doesn't properly tell us whether it > > was found or not. Using a parameter struct to pass back this value to > > the caller feels like a code smell, and only papers over this weird > > usage. > >=20 > > Anyway, your patch doesn't make it any worse, so we may just as well fix > > it after this series has landed. >=20 > I guess you wrote this before taknig a look at the next patch in this > series, which does get rid of have_it (renamed to found_uuid). Indeed. > The > intent of this patch was not to change the existing behavior, just get > rid of the globals. I could've moved the following patch before this > one, in which case have_it wouldn't exist here. But for historical > reasons it was easier not too. I don't particularly mind the order, so your current version is good enough for me. > >=20 > > [snip] > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > > > index 5bd970692..230167ab0 100644 > > > --- a/include/grub/cryptodisk.h > > > +++ b/include/grub/cryptodisk.h > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t > > > =20 > > > struct grub_cryptomount_args > > > { >=20 > /* Flag to indicate that only bootable volumes should be decrypted */= =20 >=20 > > > + grub_uint32_t check_boot : 1; > > > + grub_uint32_t found_uuid : 1; >=20 > /* Only volumes matching this UUID should be decrpyted */ >=20 > > > + char *search_uuid; >=20 > /* Key data used to decrypt voume */ This can either contain the user-provided password or contents of a key file, right? Might be worthwhile to document that. > > > grub_uint8_t *key_data; >=20 > /* Length of key_data */ >=20 > > > grub_size_t key_len; > > > }; > >=20 > > Would be nice to have comments here which explain what these parameters > > do. for `key_data` and `key_len` it's obvious enough, but as a reader I > > wouldn't really know what `check_boot` ought to indicate. >=20 > I was trying to use the same names to provide continuity (except for > have_it which is badly named). check_boot might better be named as > only_boot. I can add some comments. I agree that as a reader I like to > see descriptions of struct fields. How do the comments above look? Thanks. Except for the one addendum above they look nice to me. > I > didn't add one for found_uuid because the next patch gets rid of it > anyway. >=20 > Glenn Fair enough. Patrick --uhOx/5+z0BE9lHK7 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEEF9hrgiFbCdvenl/rVbJhu7ckPpQFAmFavsUACgkQVbJhu7ck PpRhnBAAln6uhpKnRDQz1OnTdaA2m7If2kJrHJqkxWg4loSND0AlC7W/yiPvaLxE 3ydA3uBdd5A4NIZxCRiss1/Y8skyAOG0jPfxcmWPKDD3E7PoDm237mCSp6H96JNp aGEf1h4MJrB+YfCEYYAHJnFdGnVP7bcMwWb8ZKqngEnsMc1RtjKeynTgkdnx+iUI J3ka5AiPuA9q6HwDlqLKEFVVM/byV9mRczxRvrtKYYq5/Pyl2gOEm2rE92GxgWIl 6AF3qb41SY9VzPC/nYPtzT1s3pu3rARSbx79pX/DT9uC3raPRDfOI+LlY73GaLnP 0trEEu+G51cdnMeetR6VuM0xVCvg9Ljf7wwynHi7SgPwtZYVKB4FM8EzpbHA8FUg Jft0GIhdtqF8bFM4RZfQB1ilLiLu11+GUfIVXO5gqtaLU1WBLlPD+ykj7Hoc+inP i1xi+kUqs6Nf0jqPp+rifJlehC75yovim89Z5pkc9imBcVzqWO8pz+0sLlbl/3Hr gjxyaR2dBnbVZhs0izkGCpZz/M4SW9t96GKX0XVtCMTZQSQy3u1ijDfUHTwftq52 AQOzp76vB2zuZ2Y5NVpLpVkFkuqsOkm70ezGqZ8hzQEMKzwaIR5EK4DCbjsWTFJZ yNSEe14ApLzzgzICYxn4T102M5EojzDGF2lqqs66Hr4w+1Ihflc= =jB0H -----END PGP SIGNATURE----- --uhOx/5+z0BE9lHK7--