Re: [PATCH v4 22/32] libxl_qmp: Handle messages from QEMU

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Jul 27, 2018 at 03:06:04PM +0100, Anthony PERARD wrote:
> This will handles messages received, and calls callbacks associated with
            ^ handle

I'm not sure I understand what's 'This' in the context. Would be good
if you could spell out what 'This' refers to IMO.

> the libxl__ev_qmp when the expected response is received.
> 
> This also print error messages from QMP on behalf of the callback.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> 
> Notes:
>     v4:
>         Provide an libxl error code to callbacks on error instead of a
>         qmp_error_class
> 
>  tools/libxl/libxl_qmp.c              | 116 +++++++++++++++++++++++++++
>  tools/libxl/libxl_types.idl          |   4 +
>  tools/libxl/libxl_types_internal.idl |   8 ++
>  3 files changed, 128 insertions(+)
> 
> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> index aabf9ad5e7..e649b8054d 100644
> --- a/tools/libxl/libxl_qmp.c
> +++ b/tools/libxl/libxl_qmp.c
> @@ -1330,6 +1330,118 @@ static int qmp_ev_prepare_cmd(libxl__gc *gc,
>      return 0;
>  }
>  
> +/*
> + * Handle messages received from QMP server
> + */
> +
> +static int qmp_error_class_to_libxl_error_code(const libxl__qmp_error_class c)
> +{
> +    switch (c) {
> +    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
> +        return ERROR_QMP_GENERIC_ERROR;
> +    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
> +        return ERROR_QMP_COMMAND_NOT_FOUND;
> +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTACTIVE:
> +        return ERROR_QMP_DEVICE_NOT_ACTIVE;
> +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
> +        return ERROR_QMP_DEVICE_NOT_FOUND;
> +    default:
> +        abort();
> +    }
> +}
> +
> +/* return 1 when a user callback as been called */
> +static int qmp_ev_handle_response(libxl__egc *egc,
> +                                  libxl__ev_qmp *ev,
> +                                  const libxl__json_object *resp,
> +                                  libxl__qmp_message_type type)
> +{
> +    EGC_GC;
> +    const libxl__json_object *response;
> +    const libxl__json_object *o;
> +    int rc;
> +    int id;
> +
> +    o = libxl__json_map_get("id", resp, JSON_INTEGER);
> +    if (!o) {
> +        const char *error_desc;
> +
> +        /* unexpected message, attempt to find an error description */
> +        o = libxl__json_map_get("error", resp, JSON_MAP);
> +        o = libxl__json_map_get("desc", o, JSON_STRING);
> +        error_desc = libxl__json_object_get_string(o);
> +        if (error_desc)
> +            LOGD(ERROR, ev->domid, "%s", error_desc);
> +        else
> +            LOGD(ERROR, ev->domid, "Received unexpected message: %s",
> +                 libxl__json_object_to_json(gc, resp));
> +        return 0;
> +    }
> +
> +    id = libxl__json_object_get_integer(o);
> +    if (id != ev->id)
> +        return 0;
> +
> +    switch (type) {
> +    case LIBXL__QMP_MESSAGE_TYPE_RETURN: {
> +        response = libxl__json_map_get("return", resp, JSON_ANY);
> +        rc = 0;
> +        break;
> +    }
> +    case LIBXL__QMP_MESSAGE_TYPE_ERROR: {
> +        const char *s;
> +        const libxl__json_object *err;
> +        libxl__qmp_error_class error_class;
> +
> +        rc = ERROR_FAIL;
> +        response = NULL;
> +
> +        err = libxl__json_map_get("error", resp, JSON_MAP);
> +        o = libxl__json_map_get("class", err, JSON_STRING);
> +        s = libxl__json_object_get_string(o);

You could init err and s at definition time.

> +        if (s && !libxl__qmp_error_class_from_string(s, &error_class))
> +            rc = qmp_error_class_to_libxl_error_code(error_class);
> +
> +        o = libxl__json_map_get("desc", err, JSON_STRING);
> +        s = libxl__json_object_get_string(o);
> +        if (s)
> +            LOGD(ERROR, ev->domid, "%s", s);
> +
> +        break;
> +    }
> +    default:
> +        abort();
> +    }
> +
> +    ev->id = -1;
> +    ev->callback(egc, ev, response, rc); /* must be last */
> +    return 1;
> +}
> +
> +/* return 1 when a user callback as been called */
> +static int qmp_ev_handle_message(libxl__egc *egc,
> +                                 libxl__ev_qmp *ev,
> +                                 const libxl__json_object *resp)
> +{
> +    EGC_GC;
> +    libxl__qmp_message_type type = qmp_response_type(resp);
> +
> +    switch (type) {
> +    case LIBXL__QMP_MESSAGE_TYPE_QMP:
> +        /* greeting message */
> +        return 0;
> +    case LIBXL__QMP_MESSAGE_TYPE_RETURN:
> +    case LIBXL__QMP_MESSAGE_TYPE_ERROR:
> +        return qmp_ev_handle_response(egc, ev, resp, type);
> +    case LIBXL__QMP_MESSAGE_TYPE_EVENT:
> +        /* Event are ignored */
> +        return 0;
> +    case LIBXL__QMP_MESSAGE_TYPE_INVALID:
> +        return 0;

Might be good to have a 'default' lable here and print some debug
message about receiving an unknown QMP message type?

> +    }
> +    return 0;
> +}
> +
>  /*
>   * QMP FD callbacks
>   */
> @@ -1432,6 +1544,10 @@ static int qmp_ev_callback_readable(libxl__egc *egc, libxl__ev_qmp *ev, int fd)
>              end = NULL;
>  
>          LOG_QMP("JSON object received: %s", libxl__json_object_to_json(gc, o));
> +
> +        /* Must be last and return when the user callback is called */
> +        if (qmp_ev_handle_message(egc, ev, o) == 1)
> +            return 0;
>      }
>      return 0;
>  }
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 4a385801ba..e104fea941 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -69,6 +69,10 @@ libxl_error = Enumeration("error", [
>      (-23, "NOTFOUND"),
>      (-24, "DOMAIN_DESTROYED"), # Target domain ceased to exist during op
>      (-25, "FEATURE_REMOVED"), # For functionality that has been removed
> +    (-26, "QMP_GENERIC_ERROR"), # unspecified qmp error
> +    (-27, "QMP_COMMAND_NOT_FOUND"), # the requested command has not been found
> +    (-28, "QMP_DEVICE_NOT_ACTIVE"), # a device has failed to be become active
> +    (-29, "QMP_DEVICE_NOT_FOUND"), # the requested device has not been found

Do we really need such granularity for QMP errors? Isn't it enough to
have a single ERROR_QMP or similar?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel