Re: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key

[James Bottomley]

On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote:
> James,
> 
> I like the improvements here. However, I've been thinking more about
> this and other improvements that deal with passing parameters to
> modules used by cryptomount. I have some ideas that I've not had the
> time to fully investigate or code up proof of concepts. One idea is
> that we shouldn't be changing the function declaration of recover
> key, that is to say adding new parameters. Instead we should be
> adding the parameters to grub_cryptodisk_t and setting them in
> grub_cryptodisk_scan_device_real. This would satisfy needs of this
> patch series as well as the key file, detached header, sending
> password as cryptomount arg, and master key features without
> cluttering the function signature.

Keeping large amounts of shared state between caller and callee can be
a debugger's nightmare.  In this case the only consumer of the password
callback is the recover function, so it seems appropriate it should be
an argument to that function.

> So, I don't think this is the right approach.

The thing this patch demonstrates is that altering the function
signatures is fairly easy, so it would be a simple patch to alter them
again if the password callback were decided to be an essential
component of the cryptodisk device ... but it should really driven the
need which isn't apparent yet.

James