Re: [PATCH] xen: look for Xen notes in section headers too

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]

On Thu, Mar 14, 2019 at 01:39:43PM +0100, Daniel Kiper wrote:
> On Thu, Mar 14, 2019 at 02:57:08AM +0100, Marek Marczykowski-Górecki wrote:
> > Mirror behaviour of elf loader in libxc: first look for Xen notes in
> > PT_NOTE segment, then in SHT_NOTE section and only then fallback to
> > a section with __xen_guest name.
> > This fixes loading PV kernels that Xen note have outside of PT_NOTE.
> > While this may be result of a buggy linker script, loading such kernel
> > directly works fine, so make it work with grub too.
> > Specifically, this applies to binaries built from Unikraft.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> 
> I treat this as a fix, so, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> 
> However, ...
> 
> > ---
> >  grub-core/loader/i386/xen_fileXX.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
> > index fb66e66fe..6af6cc0da 100644
> > --- a/grub-core/loader/i386/xen_fileXX.c
> > +++ b/grub-core/loader/i386/xen_fileXX.c
> > @@ -344,6 +344,21 @@ grub_xen_get_infoXX (grub_elf_t elf, struct grub_xen_file_info *xi)
> >    s = (Elf_Shdr *) ((char *) s0 + elf->ehdr.ehdrXX.e_shstrndx * shentsize);
> >    stroff = s->sh_offset;
> >
> > +  for (s = s0; s < (Elf_Shdr *) ((char *) s0 + shnum * shentsize);
> > +       s = (Elf_Shdr *) ((char *) s + shentsize))
> > +    {
> > +      if (s->sh_type == SHT_NOTE) {
> 
> Curly bracket is in wrong place...
> 
> > +	err = parse_note (elf, xi, s->sh_offset, s->sh_size);
> > +	if (err)
> > +	  goto cleanup;
> > +      }
> 
> ... and this is also in wrong place.
> 
> > +    }
> > +
> > +  if (xi->has_note) {
> > +    err = GRUB_ERR_NONE;
> > +    goto cleanup;
> > +  }
> 
> Ditto. Next time please double check that you have correct formatting.
> Now I will fix it before the push. If there are no objections then it
> will happen at the beginning of next week.

Thanks. This code style is very different than any other project I
contribute to... 

Anyway, there is a comment earlier in this function:

    /* FIXME: check note.  */

Is it about this very issue and now can be removed?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]