From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mXRXW-0003Dv-1N for mharc-grub-devel@gnu.org; Mon, 04 Oct 2021 13:14:10 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:37694) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXRXQ-0003Bu-Hl for grub-devel@gnu.org; Mon, 04 Oct 2021 13:14:04 -0400 Received: from mail-qk1-x735.google.com ([2607:f8b0:4864:20::735]:36373) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mXRXN-00034d-C4 for grub-devel@gnu.org; Mon, 04 Oct 2021 13:14:03 -0400 Received: by mail-qk1-x735.google.com with SMTP id p4so17182482qki.3 for ; Mon, 04 Oct 2021 10:14:00 -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=QssCsRGMiHkHvKPqXgnn97u6oU+ygl2RuCfBX+BiDuU=; b=zkZd+l7teCqSgkUJrt4hb1zV8D3Jrw/I3wdFm1q1gF6AMbcW8cnZdcTDwTiSvD7mvt GvFHsSf5aII66JFucmO7V3Zcs1PP/Q9xB/PbY4UQEv6hoHWTiT1P478I0uIWWeVkBGzW Pxgvf2AeHjsz9ZLZrwAlUbsBYH+TPEUd7T9jRkWaw8R3ZLUC1aNN4ATcSL5tTm1kKADk pWDJGT2wroTOAokxQYfiIBYompUH6GA9m1KhkgmalGwOcxFaY0kzVOqPx4XF5W+eOZsG YVpPryaz2RcZpPTOQSj4Y86cUzNzT8qhVaOfhVXObzVQSL2M+ezNmoOJF/RZEwi/edWa /EDw== 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=QssCsRGMiHkHvKPqXgnn97u6oU+ygl2RuCfBX+BiDuU=; b=0pRwRLhrz+mgWtNkBYGDNbHk4kjO65mUydxon184Y3WlkgQbSTlqwNtuNgCTY7NPzO ZGKIu+/s+PA9Ohr1IuZ+76aKESRu4z6wcAcuE2ZwKxVMCB6JNgu1X9eWytgTQHygu3Dn oQTgb209QUQfCHLP14LcE8T6XlJr0DijHciEMOwCNasy6O2MdMZ58KespgEpzMpeNtt2 WnncYe1OyWrzeDiupAZSg28xK7szHuxxDAdsZ6PTCd7V94FG5nZmuq0hzARxjNSxOu4b ck5izgvvhyQeXcJbVHSwl0e3x2yohX5tEj9LIETlp+Z+GrXqBwrzqFZ0GoZUsiBpl3tO mutw== X-Gm-Message-State: AOAM531oLP9Krk9W9Tih0HdKGEhFTyLFI7cdG7CcZWclK9b/dSMteyfC 1CvZzO+sOHIEon7Ws+YLLjakm1OUDzpRSQ== X-Google-Smtp-Source: ABdhPJwEZav/HpaUh4c/WNyvcHJEexejW9XvyutOnqHKsXILqHmoEl5FMchlwMXJBttqLsLuIl87ZQ== X-Received: by 2002:a37:4116:: with SMTP id o22mr10962011qka.496.1633367639839; Mon, 04 Oct 2021 10:13:59 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([37.218.244.251]) by smtp.gmail.com with ESMTPSA id 14sm9703784qtx.33.2021.10.04.10.13.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 04 Oct 2021 10:13:59 -0700 (PDT) Date: Mon, 4 Oct 2021 12:13:53 -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: <20211004121353.10124956@crass-HP-ZBook-15-G2> In-Reply-To: References: <20210927231403.642857-1-development@efficientek.com> <20210927231403.642857-4-development@efficientek.com> <20211003185745.454aa5f7@crass-HP-ZBook-15-G2> 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::735; envelope-from=development@efficientek.com; helo=mail-qk1-x735.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: Mon, 04 Oct 2021 17:14:05 -0000 On Mon, 4 Oct 2021 10:43:50 +0200 Patrick Steinhardt wrote: > 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: > > > > > 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). > > 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. > > > > > > > [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 */ > > This can either contain the user-provided password or contents of a key > file, right? Might be worthwhile to document that. Make sense. I'll add that in. I'm seeing now that the comment as is could lead one to believe that its the master key data, which its not. Glenn