Re: [libvirt] [PATCH] Allow to dynamically set the size of the debug buffer

On 03/08/2011 03:44 AM, Daniel Veillard wrote: > Allow to dynamically set the size of the debug buffer > > This is the part allowing to dynamically resize the debug log > buffer from it's default 64kB size. The buffer is now dynamically > allocated. > It adds a new API virLogSetBufferSize() which resizes the buffer > If passed a zero size, the buffer is deallocated and we do the small > optimization of not formatting messages which are not output anymore. Can you convince git to also include the diffstat of your patches?

> @@ -192,6 +195,15 @@ int virLogStartup(void) { > > virLogInitialized = 1; > virLogLock(); > + if (VIR_ALLOC_N(virLogBuffer, virLogSize) < 0) { > + /* > + * The debug buffer is not a critical component, allow startup > + * even in case of failure to allocate it in case of a > + * configuration mistake. > + */ > + virReportOOMError(); > + virLogSize = 0; If there was a configuration mistake, such as requesting way too much memory, should we first try a fallback of 64k before giving up completely?

Is virReportOOMError() the right thing to do here, if we are proceeding on with execution, or do we need to issue a manual VIR_WARN instead?

> /** > + * virLogSetBufferSize: > + * @size: size of the buffer in kilobytes or 0 to deactivate > + * > + * Dynamically set the size or desactivate the logging buffer used to keep s/desactivate/deactivate/ > +extern int > +virLogSetBufferSize(int size) { Why not s/int/size_t/, so that 64-bit systems with beefy RAM can request gigabytes of log? Then again, that seems like a resource hog, so int is probably okay.

> + int ret = 0; > + int oldsize; > + char *oldLogBuffer; > + > + if (size < 0) > + size = 0; > + > + if ((virLogInitialized == 0) || (size * 1024 == virLogSize)) Ouch - you didn't check for integer overflow before doing this multiply. You need to add something like:

if (INT_MAX / 1024 < size) { VIR_WARN("requested log size too large: %d", size); return -1; }

> + return ret; > + > + virLogLock(); > + > + oldsize = virLogSize; > + oldLogBuffer = virLogBuffer; > + > + virLogSize = size * 1024; > + if (VIR_ALLOC_N(virLogBuffer, virLogSize) < 0) { > + virReportOOMError(); > + virLogBuffer = oldLogBuffer; > + virLogSize = oldsize; > + ret =-1; Spacing looks awkward, s/=-1/= -1/ > @@ -350,23 +408,28 @@ virLogEmergencyDumpAll(int signum) { > virLogDumpAllFD( "Caught unexpected signal", -1); > break; > } > + if ((virLogBuffer == NULL) || (virLogSize <= 0)) { > + virLogDumpAllFD(" internal log buffer desactivated\n", -1); s/desactivated/deactivated/