On Wed, Oct 07, 2009 at 03:34:38PM -0400, Amy Griffis wrote:
Daniel P. Berrange wrote: [Tue Oct 06 2009, 05:00:55AM EDT]
Having looked at the wway the next patch uses these, I think it'd be nicer to change the contract to just be
extern char *virLogGetFilters(void); extern char *virLogGetOutputs(void);
Heh, that's how I wrote it the first time. Then I changed it to make use of the virBuffer API, and tried to follow precedent with the rest of libvirt. The code is not really doing much with the string, maybe virBuffer is overkill?
The general rule to try & follow is that if you just have a single printf style call to make, then use virAsprintf. If you have several printf/strcat calls to make, then use virBuffer.
In this particular case, its fine to use virBuffer for your internal impl - I just thing its better to not include it in the public API, convert the virBuffer into a char * for the return value
That makes sense. Does this look good now?
---
When configuring logging settings, keep more information about the output destination. Add accessors to retrieve the filter and output settings in the original string form; this to be used to set up environment for a child process that also logs. Open output files O_APPEND so child can also write -- was there a reason to truncate them?
Note this patch changes the API for virLogDefineOutput(), which is part of the internal libvirt API, but is currently only used within logging.c. ---
src/libvirt_private.syms | 2 + src/util/logging.c | 112 +++++++++++++++++++++++++++++++++++++++++++--- src/util/logging.h | 14 +++++- 3 files changed, 119 insertions(+), 9 deletions(-)
ACK, this looks good to me. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|