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

[Rob Hoes <Rob.Hoes@xxxxxxxxxx>]

> 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.

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.

> > +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).

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

I suppose we could do that indeed.
 
> (Also, "vfoobar" is a convention used by C programmers to indicate that a
> function takes a stdarg.h va_list.  You probably want to call this function
> "message".)

OK.

Rob

> > diff --git a/tools/ocaml/libs/xentoollog/xentoollog.mli
> > b/tools/ocaml/libs/xentoollog/xentoollog.mli
> > new file mode 100644
> ...
> > +type level =
> > +   | Debug
> > +   | Verbose
> > +   | Detail
> > +   | Progress (* also used for "progress" messages *)
> > +   | Info
> > +   | Notice
> > +   | Warn
> > +   | Error
> > +   | Critical
> 
> What, another copy of this ?
> 
> > diff --git a/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> > b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> > new file mode 100644
> > index 0000000..c6430b1
> > --- /dev/null
> > +++ b/tools/ocaml/libs/xentoollog/xentoollog_stubs.c
> > @@ -0,0 +1,222 @@
> ...
> > +   switch (c_level) {
> > +   case XTL_NONE: /* Not a real value */
> > +           caml_raise_sys_error(caml_copy_string("Val_level
> XTL_NONE"));
> > +           break;
> > +   case XTL_DEBUG:    return Val_int(0);
> > +   case XTL_VERBOSE:  return Val_int(1);
> > +   case XTL_DETAIL:   return Val_int(2);
> > +   case XTL_PROGRESS: return Val_int(3);
> > +   case XTL_INFO:     return Val_int(4);
> > +   case XTL_NOTICE:   return Val_int(5);
> > +   case XTL_WARN:     return Val_int(6);
> > +   case XTL_ERROR:    return Val_int(7);
> > +   case XTL_CRITICAL: return Val_int(8);
> > +   case XTL_NUM_LEVELS: /* Not a real value! */
> 
> _Another_ copy of this!
> 
> Thanks,
> Ian.

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