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

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Wed, 2011-06-15 at 19:44 +0100, Anthony PERARD wrote:
> On Wed, Jun 15, 2011 at 11:04, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> 
> wrote:
> > On Tue, 2011-06-14 at 18:15 +0100, Anthony Perard wrote:
> >> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >>
> >> QMP stands for QEMU Monitor Protocol and it is used to query information
> >> from QEMU or to control QEMU.
> >>
> >> This implementation will ask QEMU the list of chardevice and store the
> >> path to serial0 in xenstored. So we will be able to use xl console with
> >> QEMU upstream.
> >>
> >> In order to connect to the QMP server, a socket file is created in
> >> /var/run/xen/qmp-$(domid).
> >>
> >> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> >> ---
> >>
> >> Change v2->v3:
> >>   - Use of a timeout when wait for a reply from the server.
> >>   - Use of a command list, a list of pair "command" + callback. It's 
> >> associated
> >>     with an enum.
> >>   - Introduce libxl__qmp_initializations that will ask of all informations 
> >> need
> >>     through QMP. (It's just a rename of libxl__qmp_get_serial_console_path 
> >> from
> >>     the previous patch.)
> >
> > I would have sworn that initiali[zs]ations wasn't the plural of
> > initiali[zs]e (it sounds wrong to my ear) and that it was one of those
> > words with the same plural and singular form, but the Internet tells me
> > I'm wrong...
> >
> > On the other hand libxl__qmp_domain_create works, so would something
> > like libxl__qmp_gather_initial_info, the later conveys what's actually
> > going on too...
> 
> Actually, we could also set up the VNC password by this way. So in
> this case, it will not be anymore only "gather initial info".

Sure. In which case libxl__qmp_domain_create (or _setup) works.

> >> @@ -245,6 +245,12 @@ static char ** 
> >> libxl__build_device_model_args_new(libxl__gc *gc,
> >>      flexarray_vappend(dm_args, dm,
> >>                        "-xen-domid", libxl__sprintf(gc, "%d", 
> >> info->domid), NULL);
> >>
> >> +    flexarray_append(dm_args, "-qmp");
> >> +    flexarray_append(dm_args,
> >> +                     libxl__sprintf(gc, "unix:%s/qmp-%d,server,nowait",
> >> +                                    libxl_run_dir_path(),
> >> +                                    info->domid));
> >
> > Presumably qemu will clean this socket up in the normal shutdown case.
> > Do we need to do so as well to handle crashes and the like?
> 
> Actually, qemu doesn't remove the socket. So we will have do clean it
> when the domain is cleaned. Is libxl_domain_destroy() a good function
> to do that?

I think so. There must be a function somewhere which signals qemu to
shutdown (right?) -- I guess that be a good place?

> >> +        if (errno == ENOENT || errno == ECONNREFUSED) {
> >> +            /* ENOENT       : Socket may not have shown up yet
> >> +             * ECONNREFUSED : Leftover socket hasn't been removed yet */
> >> +            continue;
> >> +        }
> >> +        return -1;
> >> +    } while ((++i <= timeout * 5) && (usleep(.2 * 1000000) <= 0));
> >
> > usleep (effectively) takes an unsigned int, what typedoes .2 * x become?
> 
> It should be a float. But have 0.2 * 10^6 will be better to understand
> that we have 0.2 second of sleep, but not power in C. If you prefere,
> I will just wrote 200000, should be OK.

My concern was just the floating * int multiplication going into a
function which takes an int. I'm sure it all works out through the up-
and down-casting and constant propagation etc and turns into the right
thing, I just wondered if we could avoid it and make things clearer at
the same time.


> >> +#ifdef DEBUG_RECEIVE
> >> +    qmp->buffer[rd] = 0;
> >> +    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "received: '%s'", qmp->buffer);
> >> +#endif
> >> +
> >> +    while (bytes_parsed < rd) {
> > [...]
> >> +        /* skip the CRLF of the end of a command */
> >> +        while (bytes_parsed < rd && (qmp->buffer[bytes_parsed] ==
> >> '\r'
> >> +                                     || qmp->buffer[bytes_parsed] ==
> >> '\n')) {
> >> +               bytes_parsed++;
> >> +        }
> >
> > The parser doesn't just eat \r & \n?
> 
> It does not eat it when it finished to parse a response but it will
> eat these after. So the loop do another round and end up with a
> partiel json data and an allocated parser.
> 
> This little loop is just here to avoid doing a not necessary turn but
> is not necessary

It seems worthwhile then.

> > [...]
> >> +static int qmp_send(libxl__qmp_handler *qmp, const char *cmd, 
> >> qmp_callback_t callback)
> >> +{
> >> +    yajl_gen_config conf = { 0, NULL };
> >> +    const unsigned char *buf;
> >> +    const char *ex = "execute";
> >> +    unsigned int len = 0;
> >> +    yajl_gen_status s;
> >> +    yajl_gen hand;
> >> +
> >> +    hand = yajl_gen_alloc(&conf, NULL);
> >> +    if (!hand) {
> >> +        return -1;
> >> +    }
> >> +
> >> +    yajl_gen_map_open(hand);
> >> +    yajl_gen_string(hand, (const unsigned char *)ex, strlen(ex));
> >> +    yajl_gen_string(hand, (const unsigned char *)cmd, strlen(cmd));
> >> +    yajl_gen_string(hand, (const unsigned char *)"id", 2);
> >
> > ex is a variable and "id" is not?
> 
> Well, it was easier to cound the char in "id" than in "execute", that
> why there is a ex variable. But I think the next step would to eithier
> use a #define QMP_{EXECUTE,ID} or a static const char* global to the
> file.
> 
> > You'd think yajl would have yajl_gen_asciiz or something.
> 
> Unfortunatly, there is no such thing, at least in the debian version
> (1.?). I did not check the 2.? version of libyajl.

Perhaps we should have our own little helper function/macro which does
this?

> >> +    yajl_gen_integer(hand, ++qmp->last_id_used);
> >> +    yajl_gen_map_close(hand);
> >> +
> >> +    s = yajl_gen_get_buf(hand, &buf, &len);
> >> +
> >> +    if (s) {
> >> +        LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
> >> +                   "could not generate a qmp command");
> >> +        return -1;
> >> +    }
> >> +
> >> +    if (callback) {
> >> +        callback_id_pair *elm = malloc(sizeof (callback_id_pair));
> >> +        elm->id = qmp->last_id_used;
> >> +        elm->callback = callback;
> >> +        SIMPLEQ_INSERT_TAIL(&qmp->callback_list, elm, next);
> >> +    }
> >> +
> >> +    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "next qmp command: '%s'", buf);
> >> +
> >> +    write(qmp->qmp_fd, buf, len);
> >> +    write(qmp->qmp_fd, "\r\n", 2);
> >
> > These need to handle partial writes?
> 
> What did you mean ?
> Call write twice was easier here than call a snprintf or other.

I meant handling of write() returning < len or EINTR, EAGAIN etc.

> Thanks for the review,

No problem!

Ian.


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