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