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