From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mXBMj-0003IO-Na for mharc-grub-devel@gnu.org; Sun, 03 Oct 2021 19:57:57 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:39884) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXBMh-0003Hs-IO for grub-devel@gnu.org; Sun, 03 Oct 2021 19:57:55 -0400 Received: from mail-qt1-x82c.google.com ([2607:f8b0:4864:20::82c]:41617) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mXBMf-0003a0-1A for grub-devel@gnu.org; Sun, 03 Oct 2021 19:57:55 -0400 Received: by mail-qt1-x82c.google.com with SMTP id t2so14426443qtx.8 for ; Sun, 03 Oct 2021 16:57:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=PBRrbksltIvPIteratbqgi6FqTW2W2AaKj1JKRty7eU=; b=bP16Mbj3ag2tvwLr5qGIV/e+KP31O/RPM1JYdBJ/5XvBLXDWULlnUcy+dj4mExpS5l 9MKYvRsu2X+pgQrwuAVBOlvk+IQgPTrzB+WnxLYSAgcoGTVmr0ArSRxGUh5yxi4RXvbP ZnYl0+1PcWszgvid/E+czYqoHrubeA0AhqE469NJdNzb2uwbFgMCakJ88046gfKwqwPd QO/lwNGWTnww1aCq37Qz2jhdpcTA/HedWxudCVW0RsQd3bNZ+GOQiQnliWvqcyTmCWh4 e3p07C6ZLsQW12VIHg+r8Q3FjX5OJzB6Vv279VXtrnX0GjpvoT7Jz0l3MEotOwVnU/j+ vyRw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=PBRrbksltIvPIteratbqgi6FqTW2W2AaKj1JKRty7eU=; b=0XxmHkrw0fAw0pTW1vihPXR/fbfjDw7agFwHBM+cPctsUjVyotRH7LSqT9HeVSQ6W2 cROSttvy8QUeIeFP8i4FtL9LtwaSV2OYCuzd4qt0JveaNQBrDSSxlNoEWe1UvyAMZ9x9 DJJ4IhzDbYDv0WHBzmkaW2ajCNhKreKVIz3n6lfsV4SJiieVvRyzMWrhPuqYh3x7TXsU nIDedrREx+KBD+pfDYpx8d0a4bcZONS6C9JSZuHiJFWXJTe4ljJGTYNwEEOXmUWH7CoA m6LCJbVjxY2bk/xpXVXb2Myldw8dGjI1yfMj+72RfNI80e4eCV8jQydNpsM2Y7S3lNwx 6P9w== X-Gm-Message-State: AOAM532sJ4BRK6yzZ8NZUD/gX1CXQeafG+4lSL6FjVriq1nnPySuMmTu WZZq6TpZ2W/1GkWUdzQpJ1UOyA== X-Google-Smtp-Source: ABdhPJyX/gN56RtrIe7lSujply28FF8w9pKafzPV3rr/EwhKo+2qaPcsjD1pzOJujb7xDTF+mpdr6A== X-Received: by 2002:a05:622a:1a0b:: with SMTP id f11mr10424798qtb.194.1633305471878; Sun, 03 Oct 2021 16:57:51 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([37.218.244.251]) by smtp.gmail.com with ESMTPSA id f5sm6756315qkk.96.2021.10.03.16.57.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Oct 2021 16:57:51 -0700 (PDT) Date: Sun, 3 Oct 2021 18:57:45 -0500 From: Glenn Washburn To: Patrick Steinhardt 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: <20211003185745.454aa5f7@crass-HP-ZBook-15-G2> In-Reply-To: References: <20210927231403.642857-1-development@efficientek.com> <20210927231403.642857-4-development@efficientek.com> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.33; 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::82c; envelope-from=development@efficientek.com; helo=mail-qt1-x82c.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: Sun, 03 Oct 2021 23:57:55 -0000 On Sun, 3 Oct 2021 21:16:09 +0200 Patrick Steinhardt wrote: > 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(-) > > > > 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) > > > > #endif > > > > -static int check_boot, have_it; > > 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. > > Anyway, your patch doesn't make it any worse, so we may just as well fix > it after this series has landed. 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). 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. > > [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 > > > > struct grub_cryptomount_args > > { /* Flag to indicate that only bootable volumes should be decrypted */ > > + grub_uint32_t check_boot : 1; > > + grub_uint32_t found_uuid : 1; /* Only volumes matching this UUID should be decrpyted */ > > + char *search_uuid; /* Key data used to decrypt voume */ > > grub_uint8_t *key_data; /* Length of key_data */ > > grub_size_t key_len; > > }; > > 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. 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? I didn't add one for found_uuid because the next patch gets rid of it anyway. Glenn