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

9 Mar 2011


      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@veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/

    