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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list