[libvirt] [PATCH] virBitmapFree: Change the function to a macro

[Daniel P. Berrange]

On Mon, Sep 09, 2013 at 01:39:42AM +0000, Liuji (Jeremy) wrote:
> > -----Original Message-----
> > From: Daniel P. Berrange [mailto:berrange at redhat.com]
> > Sent: Friday, September 06, 2013 6:37 PM
> > To: Liuji (Jeremy)
> > Cc: libvir-list at redhat.com; Jinbo (Justin); Luohao (brian); Haofeng
> > Subject: Re: [libvirt] [PATCH] virBitmapFree: Change the function to a macro
> > 
> > On Fri, Sep 06, 2013 at 10:30:56AM +0000, Liuji (Jeremy) wrote:
> > > The parameter of virBitmapFree function is just a pointer, not a pointer of
> > pointer.
> > > The second VIR_FREE on virBitmapFree only assign NULL to the formal
> > parameter.
> > > After calling the virBitmapFree function, the actual parameter are still not
> > NULL.
> > > There are many code segment don't assign NULL to the formal parameter
> > after calling
> > > the virBitmapFree function. This will bring potential risks.
> > >
> > > A problem scenario:
> > > 1) The XML of VM contain the below segment:
> > >     <numatune>
> > >         <memory mode='preferred' placement='auto' nodeset='0'/>
> > >     </numatune>
> > > 2)virsh create the VM
> > > 3)In the virDomainDefParseXML funtion:
> > >                     /* Ignore 'nodeset' if 'placement' is 'auto' finally */
> > >                     if (placement_mode ==
> > VIR_NUMA_TUNE_MEM_PLACEMENT_MODE_AUTO) {
> > >
> > virBitmapFree(def->numatune.memory.nodemask);
> > >                         def->numatune.memory.nodemask = NULL;
> > >                     }
> > > 4)Then, virsh destroy the VM. In the virDomainDefFree funtion, it also call the
> > > virBitmapFree function to free the nodemask:
> > >     virBitmapFree(def->numatune.memory.nodemask);
> > > But after this call, the value of def->numatune.memory.nodemask is still not
> > NULL.
> > > This will generate an exception.
> > 
> > Have you got an actual crash happening today, or is this just a theoretical
> > problem you're trying to address ?
> 
> Yes，it's an actual crash problem. I found the problem in the above problem scenario in my first mail.

Then can you send a patch which explicitly fixes that problem on its
own, without doing this major refactoring, which obscures what is being
fixed.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|