On Wed, Dec 01, 2021 at 03:18:06PM -0600, Glenn Washburn wrote:
> On Wed, 17 Nov 2021 18:29:36 +0100
> Daniel Kiper <dki...@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 <developm...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
