Re: [Xen-devel] [PATCH v2-resend 10/30] libxc: ocaml: add simple binding for xentoollog (output only).

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2013-08-29 at 13:54 +0100, Rob Hoes wrote:
> > Rob Hoes writes ("[Xen-devel] [PATCH v2-resend 10/30] libxc: ocaml: add
> > simple binding for xentoollog (output only)."):
> > > These bindings allow ocaml code to receive log message via xentoollog
> > > but do not support injecting messages into xentoollog from ocaml.
> > > Receiving log messages from libx{c,l} and forwarding them to ocaml is
> > > the use case which is needed by the following patches.
> > ...
> > > +type level = Debug
> > > + | Verbose
> > > + | Detail
> > > + | Progress
> > > + | Info
> > > + | Notice
> > > + | Warn
> > > + | Error
> > > + | Critical
> > 
> > This (and the next two stanzas too) needs to be autogenerated somehow
> > from the list in xentoollog.h.  Otherwise people will add levels in
> > xentoollog.h and the ocaml code will go wrong.
> 
> This would have been quite easy if the debug levels were part of the
> libxl IDL. Unfortunately they aren't, because xentoollog is part of
> libxc. So what options do we have? Adding some sort of IDL to libxc
> would be one. Or indeed parsing xentoollog.h to derive the log levels,
> but I think the risk of that going wrong due to changes in
> xentoollog.h may defeat its purpose.

I think with a sufficiently large comment in the vicinity of the enum
xentoollog_level we might be able to get away with this. Realistically
that header hasn't changed since 2010 (in fact it only changed once
meaningfully since it was added!)

> Any update to the OCaml level type would likely require the
> application that uses the bindings to be modified, so some work will
> still be needed. Perhaps we could map any new log levels to a
> well-defined "unknown" value, so that the higher level OCaml code can
> mark them as such, signalling a need to update the bindings? At least
> nothing would break horribly in this way.

This sounds like a good belt and braces thing to do regardless.

> > > +external _create_logger: (string * string) -> handle =
> > "stub_xtl_create_logger"
> > > +external test: handle -> unit = "stub_xtl_test"
> > > +
> > > +let create name cbs : handle =
> > > + (* Callback names are supposed to be unique *)
> > > + let suffix = string_of_int (Random.int 1000000) in
> > 
> > Surely this can't be a good way to go about it.
> > (Won't this fail at least one time in 1E6 ?)
> 
> Most likely this function would be called just once, or at most a few
> times, in the lifetime of the application, so it won't be that bad.
> But it would probably be better to just use a counter instead of a
> random value, and raise a proper exception when we hit 1e6 (or some
> other larger number).

I can't imagine what I was thinking here! Perhaps I couldn't figure out
how to do the ocaml equivalent of "static int counter" so I bodged it
and forgot to come back...

I think a counter would be fine. Either that or push it onto the caller
to provide something it knows is unique, but that sucks...

> > > +let stdio_vmessage min_level level errno ctx msg =
> > > + let level_str = level_to_string level
> > > + and errno_str = match errno with None -> "" | Some s -> sprintf ":
> > errno=%d" s
> > > + and ctx_str = match ctx with None -> "" | Some s -> sprintf ": %s" s in
> > > + if compare min_level level <= 0 then begin
> > > +         printf "%s%s%s: %s\n" level_str ctx_str errno_str msg;
> > > +         flush stdout;
> > > + end
> > 
> > Why are you reimplementing in ocaml the stdio logging support from
> > xenttoollog ?  Surely you'd want to simply call
> > xtl_createlogger_stdiostream ?

It was supposed to serve as a proof of concept for writing an output
module in ocaml rather than C, i.e. that the callbacks all worked right
etc. It would probably be best moved to the test app.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel