Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight

[Nick Rosbrook <rosbrookn@xxxxxxxxx>]

On Fri, Jun 18, 2021 at 01:17:07PM +0000, George Dunlap wrote:
> 
> 
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@xxxxxxxxx> wrote:
> > 
> > Add some logging methods to Context to provide easy use of the
> > Contenxt's xentoollog_logger. These are not exported, but the LogLevel
> > type is so that a later commit can allow the Context's log level to be
> > configurable.
> > 
> > Becuase cgo does not support calling C functions with variable
> > arguments, e.g. xtl_log, add an xtl_log_wrap function to the cgo preamble
> > that accepts an already formatted string, and handle the formatting in
> > Go.
> > 
> > Signed-off-by: Nick Rosbrook <rosbrookn@xxxxxxxxxxxx>
> 
> Looks good.  One comment:
> 
> > ---
> > tools/golang/xenlight/xenlight.go | 45 +++++++++++++++++++++++++++++++
> > 1 file changed, 45 insertions(+)
> > 
> > diff --git a/tools/golang/xenlight/xenlight.go 
> > b/tools/golang/xenlight/xenlight.go
> > index fc3eb0bf3f..f68d7b6e97 100644
> > --- a/tools/golang/xenlight/xenlight.go
> > +++ b/tools/golang/xenlight/xenlight.go
> > @@ -32,6 +32,15 @@ static const libxl_childproc_hooks childproc_hooks = { 
> > .chldowner = libxl_sigchl
> > void xenlight_set_chldproc(libxl_ctx *ctx) {
> >     libxl_childproc_setmode(ctx, &childproc_hooks, NULL);
> > }
> > +
> > +void xtl_log_wrap(struct xentoollog_logger *logger,
> > +             xentoollog_level level,
> > +             int errnoval,
> > +             const char *context,
> > +             const char *msg)
> > +{
> > +    xtl_log(logger, level, errnoval, context, "%s", msg);
> > +}
> > */
> > import "C"
> > 
> > @@ -192,6 +201,42 @@ func (ctx *Context) Close() error {
> >     return nil
> > }
> > 
> > +// LogLevel represents an xentoollog_level, and can be used to configre 
> > the log
> > +// level of a Context's logger.
> > +type LogLevel int
> > +
> > +const (
> > +   //LogLevelNone     LogLevel = C.XTL_NONE
> 
> Why are we not defining this one?  Don’t we want to be able to disable 
> logging entirely?

Hm, I'm not sure. I'll poke around to see if I had a reason for this,
otherwise I will un-comment this line.

> 
> > +   LogLevelDebug    LogLevel = C.XTL_DEBUG
> > +   LogLevelVerbose  LogLevel = C.XTL_VERBOSE
> > +   LogLevelDetail   LogLevel = C.XTL_DETAIL
> > +   LogLevelProgress LogLevel = C.XTL_PROGRESS
> > +   LogLevelInfo     LogLevel = C.XTL_INFO
> > +   LogLevelNotice   LogLevel = C.XTL_NOTICE
> > +   LogLevelWarn     LogLevel = C.XTL_WARN
> > +   LogLevelError    LogLevel = C.XTL_ERROR
> > +   LogLevelCritical LogLevel = C.XTL_CRITICAL
> > +   //LogLevelNumLevels LogLevel = C.XTL_NUM_LEVELS
> > +)
> > +
> > +func (ctx *Context) log(lvl LogLevel, errnoval int, format string, a 
> > ...interface{}) {
> > +   msg := C.CString(fmt.Sprintf(format, a...))
> > +   defer C.free(unsafe.Pointer(msg))
> > +   context := C.CString("xenlight")
> > +   defer C.free(unsafe.Pointer(context))
> 
> Hmm, allocating and freeing a fixed string every time seems pretty wasteful.  
> Would it make more sense to either use a static C string in the CGo code at 
> the top instead?  Or if not, to make context a global variable we allocate 
> once at the package level and re-use?

You're right, we should probably define a static C string in the
preamble.
> 
> Also, is ‘xenlight’ informative enough?  I haven’t looked at the other 
> “context” strings; would “go-xenlight” or something be better?
> 

I believe libxl uses "libxl." I would be fine with "go-xenlight" if you
prefer that. 

> > +
> > +   C.xtl_log_wrap((*C.xentoollog_logger)(unsafe.Pointer(ctx.logger)),
> > +           C.xentoollog_level(lvl), C.int(errnoval), context, msg)
> > +}
> 
> I think we want to make it possible long-term to configure your own logger or 
> have no logger at all; so maybe we should add a `if (ctx.logger == nil) 
> return;` at then top?
> 
Yeah, that's a good idea.

> Other than that looks good, thanks!
> 
>  -George

Thanks,
NR