Hi Ilias,
Thanks for the review!
On Thu, Feb 20, 2025 at 2:55 AM Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi Gabriel,
>
> On Mon, 17 Feb 2025 at 20:31, Gabriel Dalimonte
> <gabriel.dalimo...@gmail.com> wrote:
> >
> > In order to support renaming via SetInfo(), path must allow for longer
> > values than what was originally present when file_handle was allocated.
> >
> > Signed-off-by: Gabriel Dalimonte <gabriel.dalimo...@gmail.com>
> > ---
> >
> > (no changes since v1)
> >
> > lib/efi_loader/efi_file.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 201fa5f8f3c..6b15c1f3d27 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -40,7 +40,7 @@ struct file_handle {
> > struct fs_dir_stream *dirs;
> > struct fs_dirent *dent;
> >
> > - char path[0];
> > + char *path;
> > };
> > #define to_fh(x) container_of(x, struct file_handle, base)
> >
> > @@ -178,6 +178,7 @@ static struct efi_file_handle *file_open(struct
> > file_system *fs,
> > u64 attributes)
> > {
> > struct file_handle *fh;
> > + char *path;
> > char f0[MAX_UTF8_PER_UTF16] = {0};
> > int plen = 0;
> > int flen = 0;
> > @@ -194,11 +195,13 @@ static struct efi_file_handle *file_open(struct
> > file_system *fs,
> > plen = strlen(parent->path) + 1;
> > }
> >
> > + fh = calloc(1, sizeof(*fh));
> > /* +2 is for null and '/' */
> > - fh = calloc(1, sizeof(*fh) + plen + (flen * MAX_UTF8_PER_UTF16) +
> > 2);
> > - if (!fh)
> > - return NULL;
> > + path = calloc(1, plen + (flen * MAX_UTF8_PER_UTF16) + 2);
>
> I assume the new allocation gets freed in patch #6 right?
> Is there an easy way to include this change in the current patch?
> Otherwise, we'll end up not freeing memory on a specific sha commit.
> I can live with it if the change is too tricky though.
The `path` variable is assigned to `fh->path`
>
> > + if (!fh || !path)
> > + goto error;
> >
> > + fh->path = path;
here. I used the intermediate `path` variable to collapse calloc
failure handling
into a single conditional. If it is preferable to split the
conditional and eliminate
`path`, I can make that change.
> > fh->open_mode = open_mode;
> > fh->base = efi_file_handle_protocol;
> > fh->fs = fs;
> > @@ -245,6 +248,7 @@ static struct efi_file_handle *file_open(struct
> > file_system *fs,
> > return &fh->base;
> >
> > error:
> > + free(fh->path);
> > free(fh);
> > return NULL;
> > }
> > @@ -368,6 +372,7 @@ out:
> > static efi_status_t file_close(struct file_handle *fh)
> > {
> > fs_closedir(fh->dirs);
> > + free(fh->path);
> > free(fh);
> > return EFI_SUCCESS;
> > }
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias
Thanks,
Gabriel
