[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] [Xen-devel] Re: [PATCH V7 7/7] libxl, Introduce a QMP client
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. > + 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? > +/* > + * 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. > + 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. > +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. > + 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!) > + 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"? > + } 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. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxx http://lists.xensource.com/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |