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@xxxxxxxxxx>
> > > >
> > > > 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.