[libvirt] [PATCH] 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 (possibly dynamically but in that case the existing content is lost - the complexity wasn't looking like worth it). If passed a zero size, the buffer is deallocated and we do the small optimization of not formatting messages which are not output anymore. On the daemon side, it just adds a new option log_buffer_size to libvirtd.conf and call virLogSetBufferSize() if needed (minor pbm of the GET_CONF_* macros is you can't guess if the value was set or not) Seems to work fine, I tried to keep the code as simple as needed, this allowed me to find out that on a quiet daemon startup we emit half a megabyte of debug log ! The next patch missing now is the update of the documentation for the log buffer. 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/
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/ -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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/
On Wed, Mar 09, 2011 at 08:38:53AM +0800, Daniel Veillard wrote:
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.
Still I looked and I don't see where this could go in .gitconfig git-format-patch indicates it's a --stat option, but looking at git-config man page there seems to be nothing related in the format.* options, and I doubt merge.stat or rebase.stat are really related. So no I don't know how to convince git to do this :-) 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/
On 03/09/2011 12:01 AM, Daniel Veillard wrote: [a month later - can you tell I'm trying to trim my inbox? :) ]
Can you convince git to also include the diffstat of your patches?
I didn't used git to send the mail.
Still I looked and I don't see where this could go in .gitconfig git-format-patch indicates it's a --stat option, but looking at git-config man page there seems to be nothing related in the format.* options, and I doubt merge.stat or rebase.stat are really related.
So no I don't know how to convince git to do this :-)
Maybe it is commit.status? But 'git config --help' that says it defaults to true. Also, do you have diffstat(1) installed? Beyond that, all I can suggest is asking on the git mailing list or IRC channel. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Apr 13, 2011 at 10:11:54AM -0600, Eric Blake wrote:
On 03/09/2011 12:01 AM, Daniel Veillard wrote:
[a month later - can you tell I'm trying to trim my inbox? :) ]
hehe
Can you convince git to also include the diffstat of your patches?
I didn't used git to send the mail.
Still I looked and I don't see where this could go in .gitconfig git-format-patch indicates it's a --stat option, but looking at git-config man page there seems to be nothing related in the format.* options, and I doubt merge.stat or rebase.stat are really related.
So no I don't know how to convince git to do this :-)
Maybe it is commit.status? But 'git config --help' that says it defaults to true. Also, do you have diffstat(1) installed? Beyond
I'm on the road so unable to check my normal machine but I'm pretty sure I managed to get the diffstat on some command, just not on git send-email patch set :-)
that, all I can suggest is asking on the git mailing list or IRC channel.
Okay :-) 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/
On Tue, Mar 08, 2011 at 06:44:56PM +0800, Daniel Veillard wrote:
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 452566c..d3631ec 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2720,11 +2720,16 @@ qemudSetLogging(struct qemud_server *server, virConfPtr conf, const char *filename) { int log_level = 0; + int log_buffer_size = -2; char *log_filters = NULL; char *log_outputs = NULL; char *log_file = NULL; int ret = -1;
+ GET_CONF_INT (conf, filename, log_buffer_size); + if (log_buffer_size != -2) + virLogSetBufferSize(log_buffer_size);
The possible values here seem a little odd.
0 -> sets the log buffer size == 0 -> disables == -2 -> leave at the default < 0 -> disables
IMHO this can be simplified to just
0 -> sets the log buffer size == 0 -> disables < 0 -> leave at the default
And then make virLogSetBufferSize accept size_t
/** + * virLogSetBufferSize: + * @size: size of the buffer in kilobytes or 0 to deactivate + * + * Dynamically set the size or desactivate the logging buffer used to keep + * a trace of all recent debug output. Note that the content of the buffer + * is lost if it gets reallocated. + * + * Return -1 in case of failure or 0 in case of success + */ +extern int +virLogSetBufferSize(int size) { + int ret = 0; + int oldsize; + char *oldLogBuffer; + + if (size < 0) + size = 0;
IMHO size should just be 'size_t'. We don't need to have -ve values, since '0' already indicates disabled. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Tue, Mar 08, 2011 at 03:43:50PM +0000, Daniel P. Berrange wrote:
On Tue, Mar 08, 2011 at 06:44:56PM +0800, Daniel Veillard wrote:
diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 452566c..d3631ec 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -2720,11 +2720,16 @@ qemudSetLogging(struct qemud_server *server, virConfPtr conf, const char *filename) { int log_level = 0; + int log_buffer_size = -2; char *log_filters = NULL; char *log_outputs = NULL; char *log_file = NULL; int ret = -1;
+ GET_CONF_INT (conf, filename, log_buffer_size); + if (log_buffer_size != -2) + virLogSetBufferSize(log_buffer_size);
The possible values here seem a little odd.
0 -> sets the log buffer size == 0 -> disables == -2 -> leave at the default < 0 -> disables
-2 was just a trick to try to get from GET_CONF_INT whether the configuration file had that setting set or not. I tried to hint at the problem in the comment to my patch but this just led to confusion apparently. I will just default log_buffer_size to 64 as indicated in libvirtd.conf and fix the macro to actually get the information out.
/** + * virLogSetBufferSize: + * @size: size of the buffer in kilobytes or 0 to deactivate + * + * Dynamically set the size or desactivate the logging buffer used to keep + * a trace of all recent debug output. Note that the content of the buffer + * is lost if it gets reallocated. + * + * Return -1 in case of failure or 0 in case of success + */ +extern int +virLogSetBufferSize(int size) { + int ret = 0; + int oldsize; + char *oldLogBuffer; + + if (size < 0) + size = 0;
IMHO size should just be 'size_t'. We don't need to have -ve values, since '0' already indicates disabled.
I dislike using size_t for kilobytes as it tend to carry the meaning that the value is a lenght in byte : "This type is used to represent the size of an object." this is a very good way to get people confused, and we have had byte/kb confusion in the past already. 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/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Eric Blake