Re: [Xen-devel] [PATCH V4] libxl, Introduce a QMP client

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

anthony.perard@xxxxxxxxxx writes ("[Xen-devel] [PATCH V4] libxl, Introduce a 
QMP client"):
> QMP stands for QEMU Monitor Protocol and it is used to query information
> from QEMU or to control QEMU.

Thanks for this; it's coming along.  I have some comments....

Firstly, can you make sure all your lines are less than around 75
columns ?  Otherwise they wrap badly on 80-column terminals (when
viewing the code, and when replying to emails containing diffs).

> +     echo "XEN_RUN_DIR=\"$(XEN_RUN_DIR)\"" >> $(1).tmp;               \

Can we have the introduction of XEN_RUN_DIR in a separate patch ?

> diff --git a/tools/libxl/libxl_qmp.h b/tools/libxl/libxl_qmp.h
> new file mode 100644
> index 0000000..0e142fa
> --- /dev/null
> +++ b/tools/libxl/libxl_qmp.h

Is this supposed to be an internal-only header file ?  If so it's not
clear to me why it shouldn't all be in libxl_internal.h.

> +/* QMP Command that can be send */
> +enum libxl__qmp_command_e {
> +    QMP_COMMAND_QUERY_CHARDEV,
> +    QMP_COMMAND_NUMBER,
> +};
...
> +_hidden int libxl__qmp_send_command(libxl__qmp_handler *qmp, 
> libxl__qmp_command_e command);

I'm not entirely convinced by this interface.  Wouldn't it be better
to have a specific function for each command ?  After all commands may
have arguments.  Something like:

_hidden int libxl__qmp_send_query_chardev(libxl__qmp_handler *qmp);

It's not clear from your header file what the intended calling order
of these is.  I think there should be a comment in the private header
file explaining what to do in what order.


And if we are keeping the enums:

> +typedef struct libxl__qmp_handler libxl__qmp_handler;
> +typedef enum libxl__qmp_command_e libxl__qmp_command_e;

I don't think this _e suffix is very helpful.  Just call it
libxl__qmp_command.

And our practice is not normally to declare the tag as well as the
typedef for an enum.  So I think you want just
  typedef enum { .... } libxl__qmp_command.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index c21cfe7..079aaab 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
...
> +
> +        /* TODO only call this with qemu-version == qemu-xen */
> +        libxl__qmp_cleanup(&gc, domid);

I don't think we want to add more TODOs.  (And if we did, the word we
should use is FIXME.)

The best solution is to make this function idempotent, in which case
calling it is harmless.

> +    flexarray_append(dm_args, "-chardev");
> +    flexarray_append(dm_args,
> +                     libxl__sprintf(gc, 
> "socket,id=libxl-cmd,path=%s/qmp-%d,server,nowait",
> +                                    libxl_run_dir_path(),
> +                                    info->domid));
> +
> +    flexarray_append(dm_args, "-mon");
> +    flexarray_append(dm_args, "chardev=libxl-cmd,mode=control");

Yuck, what a horrible command line interface is presented here by
qemu!  But this is not your fault :-).

> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> new file mode 100644
> index 0000000..40adf33
> --- /dev/null
> +++ b/tools/libxl/libxl_qmp.c
> @@ -0,0 +1,974 @@
...
> +typedef enum {
> +    JSON_ERROR,
> +    JSON_NULL,
> +    JSON_BOOL,
> +    JSON_INTEGER,
> +    JSON_DOUBLE,
> +    JSON_STRING,
> +    JSON_MAP,
> +    JSON_ARRAY
> +} node_type_e;
> +
> +typedef struct json_object {

It's odd to see this here in our (libxl) code.  Is this kind of thing
not provided by the json library ?

> +typedef enum {
> +    QMP_ANY,
> +    QMP_QMP,
> +    QMP_RETURN,
> +    QMP_ERROR,
> +    QMP_EVENT
> +} message_type_e;
> +
> +static struct {
> +    const char *name;
> +    message_type_e type;
> +} member_name_to_message_type[] = {
> +    { "QMP", QMP_QMP },
> +    { "return", QMP_RETURN },
> +    { "error", QMP_ERROR },
> +    { "event", QMP_EVENT },
> +    { "", QMP_ANY },
> +};

If we do need to define these, we should use a libxl idl enum (with
Ian Campbell's string-mapping-generation patch which I will apply
shortly.

> +/*
> + * json_object functions
> + */

I don't understand what these are for.  They look disturbingly like
boilerplate or at least rather verbose plumbing.

> +static int json_object_append_to(libxl__qmp_handler *qmp, json_object *obj, 
> json_object *dst)

Does our json library not provide a pile of generic facilities for
managing json objects ?  Why do we need all of this code ?

> +static yajl_callbacks callbacks = {

Oh wait, I just looked up yajl.  Why are we using a SAX-style
(callback-based) json parser ?

Surely it would be better to use a json library which simply provides
an in-memory data structure for each json object.  We don't anticipate
dealing with any huge json objects (which is the motivation for the
SAX-style approach).


I will stop with comments now because I don't want to get too bogged
down in details if we're going to change the approach.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel