Re: [PATCH v3 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Daniel Kiper]

On Wed, Dec 01, 2021 at 03:18:06PM -0600, Glenn Washburn wrote:
> On Wed, 17 Nov 2021 18:29:36 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> > > As an example, passing a password as a cryptomount argument is 
> > > implemented.
> >
> > I am not very happy with that. Splitting this into separate patch or
> > merging with patch #2 probably would not help either.
>
> Its not clear to me what action you're desiring. I don't particularly
> like it either, but haven't thought of something better. Ideas?

No, I do not have better one now. I am afraid we have to live with it.

> > > However, the backends are not implemented, so testing this will return a 
> > > not
> > > implemented error.
> >
> > The commit message lacks of explanation why this change is needed.
> > I think you can copy part of the cover letter here.
>
> I can add an explanation.

Cool! Thanks!

> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
> > >  grub-core/disk/geli.c       |  6 +++++-
> > >  grub-core/disk/luks.c       |  7 ++++++-
> > >  grub-core/disk/luks2.c      |  7 ++++++-
> > >  include/grub/cryptodisk.h   |  9 ++++++++-
> > >  5 files changed, 46 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 90f82b2d3..577942088 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> > >      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
> > >      {"all", 'a', 0, N_("Mount all."), 0, 0},
> > >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 
> > > 0},
> > > +    {"password", 'p', 0, N_("Password to open volumes."), 0, 
> > > ARG_TYPE_STRING},
> >
> > Should not you update docs/grub.texi as well?
>
> Yep, good catch. I think the doc change should be in patch #2 because
> that's where the option actually becomes useful. What do you think?

Not perfect but I am OK with it.

[...]

> > > @@ -1317,7 +1328,7 @@ GRUB_MOD_INIT (cryptodisk)
> > >  {
> > >    grub_disk_dev_register (&grub_cryptodisk_dev);
> > >    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> > > -                       N_("SOURCE|-u UUID|-a|-b"),
> > > +                       N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
> >
> > s/[-p password] <SOURCE|-u UUID|-a|-b>/<SOURCE|-u UUID|-a|-b|-p password>/?
>
> The change you're suggesting indicates that "cryptomount -u UUID -p
> password" is not correct usage of the command, only "cryptomount -p
> password". My version doesn't support that either, but it does indicate
> that "cryptomount -p password -u UUID" is an option. The idea behind my
> version is that "-p password" is optional and can be used with any of
> the other options, but not alone. So I don't believe the suggestion is
> correct.

OK, let's use your version.

Daniel