Mailing List Archive

[RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight
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@ainfosec.com>
---
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
+ 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))
+
+ C.xtl_log_wrap((*C.xentoollog_logger)(unsafe.Pointer(ctx.logger)),
+ C.xentoollog_level(lvl), C.int(errnoval), context, msg)
+}
+
+func (ctx *Context) logd(format string, a ...interface{}) {
+ ctx.log(LogLevelDebug, -1, format, a...)
+}
+
+func (ctx *Context) logw(format string, a ...interface{}) {
+ ctx.log(LogLevelWarn, -1, format, a...)
+}
+
/*
* Types: Builtins
*/
--
2.17.1
Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight [ In reply to ]
> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>

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?

> + 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?

Also, is ‘xenlight’ informative enough? I haven’t looked at the other “context” strings; would “go-xenlight” or something be better?

> +
> + 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?

Other than that looks good, thanks!

-George
Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight [ In reply to ]
> On Jun 18, 2021, at 2:17 PM, George Dunlap <George.Dunlap@citrix.com> wrote:
>
>
>
>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
>
> Looks good. One comment:

Er, sorry, turns out I had rather more than one comment. Here’s one more:

Is there any particular reason not to export the Ctx.Log[X]() functions?

-George
Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight [ In reply to ]
On Fri, Jun 18, 2021 at 01:17:07PM +0000, George Dunlap wrote:
>
>
> > On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
>
> 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
Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight [ In reply to ]
On Fri, Jun 18, 2021 at 01:21:40PM +0000, George Dunlap wrote:
>
>
> > On Jun 18, 2021, at 2:17 PM, George Dunlap <George.Dunlap@citrix.com> wrote:
> >
> >
> >
> >> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
> >
> > Looks good. One comment:
>
> Er, sorry, turns out I had rather more than one comment. Here’s one more:
>
> Is there any particular reason not to export the Ctx.Log[X]() functions?
>
No reason other than I tend to only export functions when I know they
need to be exported. My motivation for adding these at the time was to
help debug development. Would you prefer to export them then?

Thanks,
NR
Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight [ In reply to ]
> On Jun 18, 2021, at 4:17 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>
> On Fri, Jun 18, 2021 at 01:17:07PM +0000, George Dunlap wrote:
>> 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.

I think so, just so there’s no confusion if someone decides to write Python / Rust / Lua / whatever bindings.

Thanks,
-George
Re: [RESEND PATCH 07/12] golang/xenlight: add logging conveniences for within xenlight [ In reply to ]
> On Jun 18, 2021, at 4:26 PM, Nick Rosbrook <rosbrookn@gmail.com> wrote:
>
> On Fri, Jun 18, 2021 at 01:21:40PM +0000, George Dunlap wrote:
>>
>>
>>> On Jun 18, 2021, at 2:17 PM, George Dunlap <George.Dunlap@citrix.com> wrote:
>>>
>>>
>>>
>>>> On May 24, 2021, at 9:36 PM, Nick Rosbrook <rosbrookn@gmail.com> 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@ainfosec.com>
>>>
>>> Looks good. One comment:
>>
>> Er, sorry, turns out I had rather more than one comment. Here’s one more:
>>
>> Is there any particular reason not to export the Ctx.Log[X]() functions?
>>
> No reason other than I tend to only export functions when I know they
> need to be exported. My motivation for adding these at the time was to
> help debug development. Would you prefer to export them then?

I don’t have a super-strong preference. I was just thinking that xl and libxl both use the same logger, so it would make sense for whatever was built on top of xenlight to use the same logger.

But I guess we’d want the exported version to be able to pass in its own “context”; since it’s more work than just capitalizing the method names, I’d say go ahead and postpone that for now.

Thanks,
-George