On Fri, May 03, 2019 at 11:18:09AM +0200, Michal Privoznik wrote:
> On 5/3/19 11:01 AM, Erik Skultety wrote:
> > On Thu, Apr 18, 2019 at 02:11:25PM +0200, Michal Privoznik wrote:
> > > Currently, the way virBufferFreeAndReset() works is it relies on
> > > virBufferContentAndReset() to fetch the buffer content which is
> > > then freed. This works as long as there is no bug in virBuffer*
> > > implementation (not true apparently). Explicitly call free() over
> > > buffer content.
> > >
> > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > > ---
> > > src/util/virbuffer.c | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c
> > > index b2ae7963a1..ac03b15a61 100644
> > > --- a/src/util/virbuffer.c
> > > +++ b/src/util/virbuffer.c
> > > @@ -281,9 +281,8 @@ virBufferContentAndReset(virBufferPtr buf)
> > > */
> > > void virBufferFreeAndReset(virBufferPtr buf)
> > > {
> > > - char *str = virBufferContentAndReset(buf);
> > > -
> > > - VIR_FREE(str);
> > > + if (buf)
> > > + virBufferSetError(buf, 0);
> >
> > You saved 1 call to memset. I also don't see how we can break
> > virBufferContentAndReset in a way that we couldn't mess up virBufferSetError in
> > the same way. Additionally, seeing a call to virBufferSetError in a free helper
> > looks suspicious (even if it has a '0' as argument). If anything, I'd rename
> > virBuggerFreeAndReset to virBufferClean.
>
> Thing is, if 1/3 wasn't merged, then the test I'm introducing there would
> leak memory. But not with this patch merged. So from long term perspective,
> if there is ever some similar bug we won't leak any memory.
>
> Yeah, naming is hard.
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
