Re: [Libvir] [patch 1/5] iptables: fix invalid free
On Wed, 2007-03-21 at 09:39 -0400, Daniel Veillard wrote:
On Wed, Mar 21, 2007 at 01:30:00PM +0000, Mark McLoughlin wrote:
> On Wed, 2007-03-21 at 09:03 -0400, Daniel Veillard wrote:
> > On Wed, Mar 21, 2007 at 12:47:58PM +0000, Mark McLoughlin wrote:
> > > In iptablesContextNew(), make sure we don't try and free an invalid
> > > pointer if one of the iptRulesNew() fails.
> > >
> > > Signed-off-by: Mark McLoughlin <markmc(a)redhat.com>
> > >
> > > Index: libvirt/qemud/iptables.c
> > > ===================================================================
> > > --- libvirt.orig/qemud/iptables.c
> > > +++ libvirt/qemud/iptables.c
> > > @@ -496,7 +496,7 @@ iptablesContextNew(void)
> > > {
> > > iptablesContext *ctx;
> > >
> > > - if (!(ctx = (iptablesContext *) malloc(sizeof (iptablesContext))))
> > > + if (!(ctx = (iptablesContext *) calloc(1, sizeof
(iptablesContext))))
> > > return NULL;
> > >
> > > if (!(ctx->input_filter = iptRulesNew("filter",
IPTABLES_PREFIX "INPUT")))
> >
> > I usually prefer malloc + memset( , 0, ) , but this probably comes from
> > libxml2 where I replaced malloc calls with specific wrappers (and I still
> > have a TODO for this in libvirt though some part of libvirt are not linked to
> > libxml2 I guess so that may make things a bit harder)
>
> If libvirt was going to have it's own malloc() wrapper, I'd suggest
> that the wrapper should always zero out the allocated memory. Then we
> could just replace the calloc() with the wrapper.
Well, one of the things I do in my wrapper is initilize to -1 all newly
allocated bytes, allows to pinpoint relatively easilly when you assumed zeroed
memory which was not, while not paying for the initialization when not needed.
Granted libvirt CPU usage can be considered neglectible compared to otehr parts
of the infrastructure.
So:
- I think the use case is a little different - generally in libvirt,
we're only allocating very small chunks where the CPU hit for
initialisation would be negligible and would never show up on a
profile. I'd prefer to take the minor hit of zero-initialising
most/all memory for programming ease.
- If our wrappers always zero-initialise, we don't need the
"initialise to -1 when debugging" thing.
- If we rely on calloc() zero-initialising in our wrappers, we give
opportunity for libc to optimise where it knows the memory is
already initialised - e.g. where it's mmap()ing the memory
from /dev/zero
> Don't know why we'd use the libxml2 wrappers instead of
our own?
Because tehy exist, that libvirt uses libxml2, and that would integrate
the 2 memory reports
Ah, memory usage reports ... nice. Would such a report be less useful
with the two mixed together, though? e.g. I personally would just like
to see the libvirt memory usage, rather than libxml2, in such a report.
Again, though, I think libvirt has slightly different needs from a
malloc() wrapper.
> > What's the policy w.r.t. error reporting in qemud and
libvirt related daemons
> > in general ? I guess a failure to malloc or thisd kind of problems should be
> > logged somewhere, right ?
>
> Yep. In this case it would be logged either to syslog if the daemon was
> autostarting the network or returned to the user in the case where the
> user is manually starting network.
okay, I assume those reporting layers are missing ATM, right ?
Nope, it's all there.
See, in qemudAddIptablesRules() we set VIR_ERR_NO_MEMORY if
iptablesContextNew() returns NULL, qemudAutostartConfigs() calls
qemudLog() if an error is set and qemudLog(), in turn, reports to
syslog.
Cheers,
Mark.