Re: [Xen-devel] [PATCH V6 3/3] libxl, Introduce a QMP client

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Anthony PERARD writes ("[Xen-devel] [PATCH V6 3/3] libxl, Introduce a QMP 
client"):
> QMP stands for QEMU Monitor Protocol and it is used to query information
> from QEMU or to control QEMU.

>      if (dm_starting) {
> +        if (dm_info->device_model_version == LIBXL_DEVICE_MODEL_VERSION_QEMU
_XEN) {
> +            libxl__qmp_initializations(ctx, domid);
> +        }
...
> +    flexarray_append(dm_args,
> +                     libxl__sprintf(gc, "socket,id=libxl-cmd,path=%s/qmp-%d,
server,nowait",
> +                                    libxl_run_dir_path(),

Lines too long, please wrap

> +static void qmp_handle_error_response(libxl__qmp_handler *qmp,
> +                                      const libxl__json_object *resp)
> +{
> +    callback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
...
> +    if (pp) {
> +        if (pp->id == qmp->wait_for_id) {
> +            /* tell that the id have been processed */
> +            qmp->wait_for_id = 0;
> +        }
> +        SIMPLEQ_REMOVE(&qmp->callback_list, pp, callback_id_pair, next);
> +        free(pp);

I think this needs to call the callback, or the code which set up the
callback will surely just continue to wait forever.

...

Later: I see that you have a single wait_for_id.  What if multiple
callers want to use a single qmp for multiple things ?  Or is
"wait_for_id" simply the one that you're waiting on synchronously ?

> +static int qmp_next(libxl__qmp_handler *qmp)
> +{
...
> +        ret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
> +        if (ret > 0) {
> +            rd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
> +            if (rd > 0) {
> +                break;
...
> +    s = qmp->buffer;
> +    s_end = qmp->buffer + rd;
> +    while (s < s_end) {
> +        libxl__json_object *o = libxl__json_parse(qmp->ctx, &qmp->yajl_ctx,
> +                                                  &s, s_end - s);

This assumes that the response will be received in a single read().
This is not correct.  read() may return partial results; it may also
aggregate multiple writes from the sending qemu into a single read()
result.

You need to pull data into the buffer and then test the buffer for
completeness (eg by looking for the cr-lf), and split the buffer up
into packets yourself, and if there are partial packets left over
go round and read again.


Thanks,
Ian.

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