Re: [PATCH v6 4/5] efi: Enable booting unified hypervisor/kernel/initrd images

[Trammell Hudson <hudson@xxxxxxxx>]

On Tuesday, September 29, 2020 11:37 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> On 21.09.2020 13:51, Trammell Hudson wrote:
> [...]
> > -   file->need_to_free = false;
>
> In patch 2 you don't bother clearing the field, presumably because
> it's static data and hence zero-filled anyway. This same assumption
> would then hold here. Or else, to be consistent, the earlier patch
> would want clearing added.

Removed.

> [...]
> > -          const char c = sect->Name[i];
> > -          const CHAR16 cw = name[i-1];
> > -          if ( cw == 0 && c == 0 )
>
> Blank line between declarations and statements please.

Added.

> [...]
> > -       if ( name[i-1] < 0 )
> > -           return -1;
>
> I'm afraid this is liable to trigger compiler warnings, for checking
> an unsigned quantity to be negative.

The compare function had strcmp() like semantics, but since it was
not used, I've modified it to just return a 0 or -1 in all cases.
So the comparison against < 0 goes away.

> Also throughout here "i-1" wants spelling "i - 1".

Fixed in both places.

> > -   /* PE32+ Subsystem type */
> > -   if ( pe->FileHeader.Machine != PE_HEADER_MACHINE )
> > -          return NULL;
>
> Comment and code don't look to be in line.

Yeah, comment had drifted out of sync as some of the other tests
moved around.  I've removed it in the v7 version.

--
Trammell