Re: [PATCH] i386-pc: build verifiers API as module

[Michael Chang]

On Thu, Mar 18, 2021 at 09:23:40AM +0000, Colin Watson wrote:
> On Thu, Mar 18, 2021 at 03:14:34PM +0800, Michael Chang via Grub-devel wrote:
> > Given no core functions on i386-pc would require verifiers to work and
> > the only consumer of the verifier API is the pgp module, it looks good
> > to me that we can move the verifiers out of the kernel image and let
> > moddep.lst to auto-load it when pgp is loaded on i386-pc platform.
> > 
> > This helps to reduce the size of core image and thus can relax the
> > tension of exploading on some i386-pc system with very short MBR gap
> > size. See also a very comprehensive summary from Colin [1] about the
> > details.
> 
> Thanks for working on this!  It's certainly awkward to have to deal with
> this sort of thing, but apparently not as awkward as I'd feared, and
> it's better than the alternative.

If I remember correctly in the past every core image size increase on
i386-pc build is counted. IMHO we should keep up with that practice at
least in reviewing new release ... 

> 
> > +AM_CONDITIONAL([COND_NOT_i386_pc], [test x$target_cpu != xi386 -o 
> > x$platform != xpc])
> 
> You could drop this and instead just do "if !COND_i386_pc" in
> grub-core/Makefile.am.

Indeed it looks superfluous to invent COND_NOT_i386_pc here. I will fix
in next patch.

> 
> > diff --git a/grub-core/kern/verifiers.c b/grub-core/kern/verifiers.c
> > index 75d7994cf..85887917d 100644
> > --- a/grub-core/kern/verifiers.c
> > +++ b/grub-core/kern/verifiers.c
> > @@ -221,8 +221,19 @@ grub_verify_string (char *str, enum 
> > grub_verify_string_type type)
> >    return GRUB_ERR_NONE;
> >  }
> >  
> > +#ifdef GRUB_MACHINE_PCBIOS
> > +GRUB_MOD_INIT(verifiers)
> > +#else
> >  void
> >  grub_verifiers_init (void)
> > +#endif
> 
> I think a comment here (or somewhere in the actual code, anyway, I don't
> mind where) would be useful so that people trying to work out what's
> going on don't have to hunt through commit logs to find out.

OK. I will add the comment to explain what's going on in next patch.

> 
> If these minor comments are fixed:
> 
> Reviewed-by: Colin Watson <cjwatson@debian.org>

Thanks. I will also add your Reviewed-by ...

Regards,
Michael

> 
> -- 
> Colin Watson (he/him)                              [cjwatson@debian.org]
>