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

[Daniel Veillard]

On Tue, Mar 08, 2011 at 08:33:50AM -0700, Eric Blake wrote:
> 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?

  I didn't used git to send the mail.

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

  I'm afraid VIR_WARN just won't work, we have the virLogLock()
virReportOOMError() will fail in the same way BTW, and that's the
main problem of that patch. I will need a v2.

> >  /**
> > + * 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.

  the base unit is kilobytes per the function doc, so IMHO not needed.

> > +    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:

  We're turning pedantic here. It's a manual configuration, and code
used a priori only once at startup.

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

  Same thing VIR_WARN would deadlock here.

> > +        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/

  thanks,

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/