Re: [libvirt] [PATCH] Fix logging in libvirt_lxc controller
Daniel P. Berrange wrote: [Wed Jun 17 2009, 06:44:31AM EDT]
On Tue, Jun 16, 2009 at 06:12:04PM -0400, Amy Griffis wrote:
> Cole Robinson wrote: [Tue Jun 16 2009, 02:44:28PM EDT]
> > On 06/16/2009 01:35 PM, Amy Griffis wrote:
> > > The lxc controller can't see libvirtd's log level setting so it
> > > needs to re-query it from the environment. The parsing code has
> > > a few users now, so I added a new function to the internal API,
> > > virLogParseDefaultPriority() along the lines of the other parse
> > > functions.
> >
> > I'd say go the extra step and add something like virLogSetFromEnv, which
> > encapsulates the duplicate getenv calls as well.
>
> I thought about that, but wanted to keep consistent behavior with
> the other two parsing routines. I think we could go ahead and
> change all of them to include the getenv(). Only minor gotcha is
> qemud wants to call virLogParseOutputs() with it's own string in
> one case. So we'd need to make the getenv() conditional on not
> getting an input string. I think this would be cleaner. What do
> you think?
I think there's valid reasons for both suggested APIs.
I'd suggest a slight variation on your original API, instead of:
+ logPrio = virLogParseDefaultPriority(debugEnv);
+ virLogSetDefaultPriority(logPrio);
Just have a 'virLogSetDefaultPriorityFromString(char *str)',
eg so it'd parse the string and then call virLogSetDefaultPriority
for you.
Since the config file parser in libvirtd is returning an integer,
it actually needs one function to get the environment setting and
convert it to integer, and then a second function to set the
value in the log module from an integer.
Also then having a virLogSetFromEnv() method which uses getenv to
fetch the vars and then calls the virLogSet... methods
But I see what you guys are saying. I'll make these changes and
send a new patch.
Thanks,
Amy