[PATCH v5 04/23] efi_loader: Show the resulting memory address from an alloc

[Simon Glass]

Hi Ilias,

On Wed, 11 Dec 2024 at 23:22, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> On Wed, 11 Dec 2024 at 17:53, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 11 Dec 2024 at 07:51, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > > On Wed, 11 Dec 2024 at 15:54, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > Update efi_allocate_pool_ext() to log the pointer returned from this
> > > > call, which can be helpful when debugging.
> > >
> > > Can you explain how logging the success is helping? I think its pretty
> > > pointless, why not log a failure?
> >
> > It shows the memory address that was allocated.
>
> Sure, but that's not what I asked.

On failure, the address is undefined, so logging it just adds
confusion. It will likely be a random number.

>
> >
> >
> > >
> > > >
> > > > Signed-off-by: Simon Glass <sjg at chromium.org>
> > > > ---
> > > >
> > > > (no changes since v3)
> > > >
> > > > Changes in v3:
> > > > - Show the returned address rather than the pointer
> > > >
> > > > Changes in v2:
> > > > - Use EFI_PRINT() instead of log_debug()
> > > >
> > > >  lib/efi_loader/efi_boottime.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > > index 080e7f78ae3..f27b3827ed2 100644
> > > > --- a/lib/efi_loader/efi_boottime.c
> > > > +++ b/lib/efi_loader/efi_boottime.c
> > > > @@ -13,6 +13,7 @@
> > > >  #include <irq_func.h>
> > > >  #include <log.h>
> > > >  #include <malloc.h>
> > > > +#include <mapmem.h>
> > > >  #include <pe.h>
> > > >  #include <time.h>
> > > >  #include <u-boot/crc.h>
> > > > @@ -511,6 +512,9 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
> > > >
> > > >         EFI_ENTRY("%d, %zu, %p", pool_type, size, buffer);
> > > >         r = efi_allocate_pool(pool_type, size, buffer);
> > > > +       if (r == EFI_SUCCESS)
> > > > +               EFI_PRINT("*buffer = %llx\n", (u64)map_to_sysmem(*buffer));
> > >
> > > buffer can be null if size == 0
> >
> > No, buffer cannot be NULL, I believe you are thinking of *buffer
> >
> > That's one reason why I prefer bufferp to buffer. It avoids this sort
> > of confusion.
>
> I personally don't find any of this confusing. Yes, I am talking about
> *buffer, but I am pretty sure this is obvious in the context of the if
> check. It can be NULL

OK, then I am missing your point. Could you expand on what you mean here?

Regards,
Simon