Re: [PATCH v2 00/10] Cryptodisk fixes for v2.06 redux

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:24PM -0500, Glenn Washburn wrote: > This is a minor update to fix patch 3, where I missed updating the format > string > type code. This was causing i386 builds to fail. Rangediff is included. Cool, thanks a lot for taking care of this! I've left a few comments here

Re: [PATCH v2 10/10] luks2: Rename source disk variabled named 'disk' to 'source' as in luks.c.

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:34PM -0500, Glenn Washburn wrote: > This makes it more obvious to the reader that the disk referred to is the > source disk, as opposed to say the disk holding the cryptodisk. Hum. I'm not sure this actually helps readability, mostly because I think that the

Re: [PATCH v2 09/10] cryptodisk: Rename offset in grub_cryptodisk_t to offset_sectors.

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:33PM -0500, Glenn Washburn wrote: > This makes it clear that the offset represents sectors, not bytes, in order > to improve readability. > > Signed-off-by: Glenn Washburn Reviewed-by: Patrick Steinhardt > --- > grub-core/disk/cryptodisk.c | 10 +- >

Re: [PATCH v2 08/10] cryptodisk: Rename total_length field in grub_cryptodisk_t to total_sectors.

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:32PM -0500, Glenn Washburn wrote: > This makes the creates an alignment with grub_disk_t naming of the same > field and is more intuitive as to how it should be used. > Signed-off-by: Glenn Washburn > --- There's something off with the commit message here. Other

Re: [PATCH v2 06/10] cryptodisk: Properly handle non-512 byte sized sectors.

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:30PM -0500, Glenn Washburn wrote: > By default, dm-crypt internally uses an IV that corresponds to 512-byte > sectors, even when a larger sector size is specified. What this means is > that when using a larger sector size, the IV is incremented every sector. >

Re: [PATCH v2 04/10] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:28PM -0500, Glenn Washburn wrote: > The total_length field is named confusingly because length usually refers to > bytes, whereas in this case its really the total number of sectors on the > device. Also counter-intuitively, grub_disk_get_size returns the total >

Re: [PATCH v2 03/10] luks2: Use more intuitive keyslot key instead of index when naming keyslot.

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:27PM -0500, Glenn Washburn wrote: > Use the keyslot key value in the keyslot json array rather than the index of > the keyslot in the json array. This is less confusing for the end user. For > example, say you have a LUKS2 device with a key in slot 1 and slot 4. When

Re: [PATCH v2 01/10] luks2: Fix use of incorrect index and some grub_error() messages.

2020-10-09 Thread Patrick Steinhardt
On Sat, Oct 03, 2020 at 05:55:25PM -0500, Glenn Washburn wrote: > When looping over the digests and segments, the loop variable is j, but the > variable i is used to index in the the digests and segments json array. The > variable i is the keyslot index. Similarly, there are several grub_error() >