[PATCH 2/3] cpukit/libmisc/rtems-fdt: Fixes leaked variable 'bf' in rtems-fdt.c

[Gedare Bloom]

On Tue, Jun 29, 2021 at 2:26 PM Joel Sherrill <joel at rtems.org> wrote:
>
> On Tue, Jun 29, 2021 at 3:15 PM Harrison Gerber <hgerber at uccs.edu> wrote:
> >
> > Joel,
> >
> > I'm not too sure what is meant by almost duplicating the base file name. The file referenced is 'rtems-fdt.c', which is found in a file called 'rtems-fdt' inside libmisc, so I thought it was best to have the entire directory path at the front and the name of the file being altered in the description. Let me know if this is not correct; I'm not totally sure what is meant here, but the rest I understand! This also is important to your response to the third patch in this series.
> >
>
> basename is a POSIX command/term for part of the path. dirname is the
> directory portion.
>
> /home/joel/source.c
>
> dirname portion -> /home/joel
> basename portion -> source.c
>
> cpukit/libmisc/rtems-fdt: Fixes leaked variable 'bf' in rtems-fdt.c
>
> I see now that the directory rtems-fdt is similar to the source file
> so it wasn't quite as redundant as I thought.
>
> How about this?
>
> cpukit/libmisc/rtems-fdt/rtems-fdt.c: close() file to avoid leaking descriptor
>
We usually just use two hops, e.g., cpukit/libmisc: close() file to
avoid leaking descriptor

whether or not to specify this is in rtems-fdt is a different choice.
We don't have a standard way yet of making these "tags" for commits to
orient the review/maintainer responsibility. however, we also will not
say cpukit for example for something in score, we just say score.
Here, I'd be happy with "libmisc/rtems-fdt: close() file to avoid
leaking descriptor"

> That makes the filename clear and doesn't waste characters with "in "
>
> Personally, if the comment needed to longer, I would be ok ditching
> the cpukit/ part but you don't need to do that for space.
+1 ditch it anyway, conciseness in the short-log is a good thing

>
> > Thanks for the quick response!
> >
>
> No problem. It takes a village to review patches. :)
>
> --joel
>
> > Harrison Gerber
> > hgerber at uccs.edu
> > 720-288-7308
> >
> > -----Original Message-----
> > From: Joel Sherrill <joel at rtems.org>
> > Sent: Tuesday, June 29, 2021 2:09 PM
> > To: Harrison Edward Gerber <gerberhe11 at gmail.com>
> > Cc: rtems-devel at rtems.org <devel at rtems.org>; Harrison Gerber <hgerber at uccs.edu>
> > Subject: Re: [PATCH 2/3] cpukit/libmisc/rtems-fdt: Fixes leaked variable 'bf' in rtems-fdt.c
> >
> > I'm ok with the fix but the commit message could be improved.
> > Something like this would be better.
> >
> > cpukit/libmisc/rtems-fdt.c : close() file to avoid leaking descriptor
> >
> > You almost duplicated the base file name and it isn't leaking the variable, it is leaking the file descriptor referenced by bf.
> >
> > --joel
> >
> > On Tue, Jun 29, 2021 at 2:54 PM Harrison Edward Gerber <gerberhe11 at gmail.com> wrote:
> > >
> > > See Also CID 1437645
> > >
> > > Closes #4297
> > > ---
> > >  cpukit/libmisc/rtems-fdt/rtems-fdt.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/cpukit/libmisc/rtems-fdt/rtems-fdt.c
> > > b/cpukit/libmisc/rtems-fdt/rtems-fdt.c
> > > index bfbc6102a2..5580d415e2 100644
> > > --- a/cpukit/libmisc/rtems-fdt/rtems-fdt.c
> > > +++ b/cpukit/libmisc/rtems-fdt/rtems-fdt.c
> > > @@ -611,6 +611,7 @@ rtems_fdt_load (const char* filename, rtems_fdt_handle* handle)
> > >      return fe;
> > >    }
> > >
> > > +  close (bf);
> > >    return 0;
> > >  }
> > >
> > > --
> > > 2.25.1
> > >
> > > _______________________________________________
> > > devel mailing list
> > > devel at rtems.org
> > > http://lists.rtems.org/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel