On Thu, 2023-04-13 at 12:17 +0100, Jonathan Cameron wrote:
> On Thu, 13 Apr 2023 17:33:28 +0800
> Hao Zeng <zeng...@kylinos.cn> wrote:
>
> > opened file processor not closed,May cause file processor leaks
>
> Patch description needs to say more on how this is fixed.
> Perhaps something like:
> "Open file descriptor not closed in error paths. Fix by replace
> open coded handling of read of whole file into a buffer with
> g_file_get_contents()"
>
> Fixes tag is part of the tag block so blank line here
>
> > Fixes: aba578bdac ("hw/cxl: CDAT Data Object Exchange
> > implementation")
> >
> An no blank line here.
>
> > Signed-off-by: Zeng Hao <zeng...@kylinos.cn>
> > Suggested-by: Philippe Mathieu-Daudé <phi...@linaro.org>
> > Suggested-by: Peter Maydell <peter.mayd...@linaro.org>
> >
> > ---
> > ChangeLog:
> > v3-v4:
> > Modify commit information,No code change.
> > v2->v3:
> > Submission of v3 on the basis of v2, based on Philippe
> > Mathieu-Daudé's suggestion
> > "Pointless bzero in g_malloc0, however this code would be
> > simplified using g_file_get_contents()."
> > v1->v2:
> > - Patch 1: No change in patch v1
> > - Patch 2: Fix the check on the return value of fread() in
> > ct3_load_cdat
> > ---
> > hw/cxl/cxl-cdat.c | 30 ++++++++----------------------
> > 1 file changed, 8 insertions(+), 22 deletions(-)
> >
> > diff --git a/hw/cxl/cxl-cdat.c b/hw/cxl/cxl-cdat.c
> > index 137abd0992..42c7c2031c 100644
> > --- a/hw/cxl/cxl-cdat.c
> > +++ b/hw/cxl/cxl-cdat.c
> > @@ -110,29 +110,17 @@ static void ct3_load_cdat(CDATObject *cdat,
> > Error **errp)
> > g_autofree CDATEntry *cdat_st = NULL;
> > uint8_t sum = 0;
> > int num_ent;
> > - int i = 0, ent = 1, file_size = 0;
> > + int i = 0, ent = 1;
> > + gsize file_size = 0;
> > CDATSubHeader *hdr;
> > - FILE *fp = NULL;
> > -
> > + GError *error = NULL;
>
> Blank line here.
>
>
> > /* Read CDAT file and create its cache */
> > - fp = fopen(cdat->filename, "r");
> > - if (!fp) {
> > - error_setg(errp, "CDAT: Unable to open file");
> > - return;
> > - }
> > -
> > - fseek(fp, 0, SEEK_END);
> > - file_size = ftell(fp);
> > - fseek(fp, 0, SEEK_SET);
> > - cdat->buf = g_malloc0(file_size);
> > -
> > - if (fread(cdat->buf, file_size, 1, fp) == 0) {
> > - error_setg(errp, "CDAT: File read failed");
> > + if (!g_file_get_contents(cdat->filename, (gchar **)&cdat->buf,
> > + &file_size, &error)) {
>
> Align parameters with start of 'cdat' (just after the opening
> bracket)
>
> > + error_setg(errp, "CDAT: File read failed: %s", error-
> > >message);
> > + g_error_free(error);
> > return;
> > }
> > -
> > - fclose(fp);
> > -
> > if (file_size < sizeof(CDATTableHeader)) {
> > error_setg(errp, "CDAT: File too short");
> > return;
> > @@ -218,7 +206,5 @@ void cxl_doe_cdat_release(CXLComponentState
> > *cxl_cstate)
> > cdat->free_cdat_table(cdat->built_buf, cdat-
> > >built_buf_len,
> > cdat->private);
> > }
> > - if (cdat->buf) {
> > - free(cdat->buf);
> > - }
> > + g_free(cdat->buf);
>
> Keep the protection if moving to g_free(). Not all paths to this
> function allocate cdat->buf
> Protection was not needed when the call was free() though.
>
> I have a followup patch that will deal with the other issues Peter
> pointed out. I'll
> send that once yours has been finalized.
>
> Thanks,
>
> Jonathan
>
>
>
> > }
>
Dear Jonathan
Thank you for taking the time to reply to my email. I appreciate
your the valuable information you have provided.
Already submitted in v5 according to the modifications.
Best regards
Hao
