Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk

[James Bottomley]

On Mon, 2022-02-07 at 17:00 +0000, Dr. David Alan Gilbert wrote:
> * James Bottomley (jejb@linux.ibm.com) wrote:
[...]
> > +static grub_err_t
> > +grub_efi_secret_put (const char *arg __attribute__((unused)), int
> > have_it,
> > +                grub_uint8_t **ptr)
> > +{
> > +  struct secret_entry *e = (struct secret_entry *)(*ptr -
> > (long)&((struct secret_entry *)0)->data);
> 
> use offsetof ?

Yes, Dov suggested that as well.  I don't really find either form more
or less readable, but I can certainly change it.

[...]
> > +static grub_err_t
> > +grub_efi_secret_find (struct efi_secret *s, grub_uint8_t
> > **secret_ptr)
> > +{
> > +  int len;
> > +  struct secret_header *h;
> > +  struct secret_entry *e;
> > +  unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
> > +
> > +  /* the area must be big enough for a guid and a u32 length */
> > +  if (s->size < sizeof (*h))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> > too small");
> > +
> > +  h = (struct secret_header *)ptr;
> > +  if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area
> > does not start with correct guid\n");
> > +  if (h->len < sizeof (*h))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> > too small\n");
> 
> These grub_error calls (and a couple later) are terminated using a \n
> which I don't think you want.

That does seem to be consistent with the rest.  I note I didn't see an
extra \n in testing, but I bet something in grub eats it.

James