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