On Thu, 21 Jul 2011, Ian Campbell wrote:
> On Wed, 2011-07-20 at 22:24 +0100, Anthony PERARD wrote:
> > 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).
>
> That path doesn't match the implementation now.
>
> Again I think I've reviewed much of this before so I only skimmed it,
> although I paid attention to the new stuff relating to pulling through
> to crlf etc.
>
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > new file mode 100644
> > index 0000000..90dba72
> > --- /dev/null
> > +++ b/tools/libxl/libxl_qmp.c
> [...]
> > +static int register_serials_chardev_callback(libxl__qmp_handler *qmp,
> > + const libxl__json_object *o)
> > +{
> > + const libxl__json_object *obj = NULL;
> > + const libxl__json_object *label = NULL;
> > + const char *s = NULL;
> > + flexarray_t *array = NULL;
> > + int index = 0;
> > + const char *chardev = NULL;
> > + int ret = 0;
> > +
> > + if ((array = json_object_get_array(o)) == NULL) {
> > + return -1;
> > + }
> > +
> > + for (index = 0; index < array->count; index++) {
> > + if (flexarray_get(array, index, (void**)&obj) != 0)
> > + break;
>
> I think a helper function to retrieve an index into a json_array would
> be helpful. I think in general exposing the internal use of flexarrays
> in this interface to callers can be avoided.
Ok, I will do this helper.
> > + label = json_object_get("label", obj, JSON_STRING);
> > + s = json_object_get_string(label);
>
> If obj is not a map label will be null but json_object_get_string will
> DTRT and return NULL so this works. But perhaps an explicit type check
> would be correct here?
Ok, I will add this explicit check.
> > +/*
> > + * Helpers
> > + */
> > +
> > +static libxl__qmp_message_type qmp_response_type(libxl__qmp_handler *qmp,
> > + const libxl__json_object
> > *o)
> > +{
> > + flexarray_t *maps = json_object_get_map(o);
> > + libxl__qmp_message_type type;
> > +
> > + if (maps) {
> > + libxl__json_map_node *node = NULL;
> > + int index = 0;
> > +
> > + for (index = 0; index < maps->count; index++) {
> > + if (flexarray_get(maps, index, (void**)&node) != 0)
> > + break;
>
> Another helper function to get the a node from a map? Or even a
> json_map_for_each_node type construct?
>
> > + if (libxl__qmp_message_type_from_string(node->map_key, &type)
> > == 0)
> > + return type;
> > + }
> > + }
> > +
> > + return LIBXL__QMP_MESSAGE_TYPE_INVALID;
> > +}
> > +
> > +static callback_id_pair *qmp_get_callback_from_id(libxl__qmp_handler *qmp,
> > + const libxl__json_object
> > *o)
> > +{
> > + const libxl__json_object *id_object = json_object_get("id", o,
> > + JSON_INTEGER);
> > + int id = -1;
> > + callback_id_pair *pp = NULL;
> > +
> > + if (id_object) {
> > + id = json_object_get_integer(id_object);
>
> Check that it is an integer? Presumably the -1 error return is never a
> valid id but it'd save a useless list walk.
Actually, the parametter JSON_INTEGER given to json_object_get ask to
explicitly return a json_object with an integer, otherwise, the function
return null.
So the get_integer will not return an error.
> > + SIMPLEQ_FOREACH(pp, &qmp->callback_list, next) {
> > + if (pp->id == id) {
> > + return pp;
> > + }
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> [...]
> > +static inline yajl_gen_status libxl__yajl_gen_asciiz(yajl_gen hand,
> > + const char *str)
> > +{
> > + return yajl_gen_string(hand, (const unsigned char *)str, strlen(str));
> > +}
>
> Belongs in libxl_json I think.
Actually, I have put it here to not expose the yajl_gen.h to all user of
libxl_internal.h.
Otherwise, yes, it does not really belong to libxl_qmp.
> > +static int qmp_next(libxl__gc *gc, libxl__qmp_handler *qmp)
> > +{
> > + ssize_t rd;
> > + char *s = NULL;
> > + char *s_end = NULL;
> > +
> > + char *incomplete = NULL;
> > + size_t incomplete_size = 0;
> > +
> > + do {
> > + fd_set rfds;
> > + int ret = 0;
> > + struct timeval timeout = {
> > + .tv_sec = qmp->timeout,
> > + .tv_usec = 0,
> > + };
> > +
> > + FD_ZERO(&rfds);
> > + FD_SET(qmp->qmp_fd, &rfds);
> > +
> > + 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 you do "if (rd == 0) continue" and the rd < 0 error handling+return
> here the error handling is nearer the error-site and hence easier to
> grok. (also you save a level of indentation, although the inner loop
> isn't wrapping in this particular case)
>
> Actually the error handling for ret = select is much the same, if you
> pull the ret==0 and ret<0 cases up after the select it easier to follow.
OK.
> > + if (rd > 0) {
> > + DEBUG_REPORT_RECEIVED(qmp->buffer, rd);
> > +
> > + do {
> > + char *end = NULL;
> > + if (incomplete) {
> > + size_t current_pos = s - incomplete;
> > + incomplete_size += rd;
> > + incomplete = libxl__realloc(gc, incomplete,
> > incomplete_size + 1);
> > + incomplete = strncat(incomplete, qmp->buffer, rd);
> > + s = incomplete + current_pos;
> > + s_end = incomplete + incomplete_size;
> > + } else {
> > + incomplete = libxl__strndup(gc, qmp->buffer, rd);
> > + incomplete_size = rd;
> > + s = incomplete;
> > + s_end = s + rd;
> > + }
> > +
> > + end = strstr(s, "\r\n");
>
> Is s definitely NULL terminated here? (yes, according to strncat(3),
> good!)
Exactly :), otherwise a strnstr would be helpfull. (I actually write one
before to use strncat.)
> > + if (end) {
> > + libxl__json_object *o = NULL;
> > +
> > + *end = '\0';
> > +
> > + o = libxl__json_parse(gc, s);
> > + s = end + 2;
> > +
> > + if (o) {
> > + qmp_handle_response(qmp, o);
> > + json_object_free(qmp->ctx, o);
> > + }
>
> "if (!o)" is now an error since you pulled up to a "\r\n"?
Indeed, I should add a log_error, even if libxl__json_parse already
print something. But I'm not sure if return -1 immediatly is a good
idee, maybe the next json object is good.
> > + } else {
> > + break;
> > + }
> > + } while (s < s_end);
> > + } else if (rd < 0) {
> > + LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR,
> > + "Socket read error");
> > + return rd;
> > + }
> > + } else if (ret == 0) {
> > + LIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR, "timeout");
> > + return -1;
> > + } else if (ret < 0) {
> > + LIBXL__LOG_ERRNO(qmp->ctx, LIBXL__LOG_ERROR, "Select error");
> > + return -1;
> > + }
> > + } while (s < s_end);
> > +
> > + return 1;
> > +}
> > +
> [...]
>
> Ian.
>
>
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home