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

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

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.

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

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

(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".)

> 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