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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 02, 2018 at 01:17:37PM +0200, Roger Pau Monné wrote:
> 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.

I guess 'patch' will do.

> > the libxl__ev_qmp when the expected response is received.
> > 
> > This also print error messages from QMP on behalf of the callback.
> > +    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.

err maybe, but not s, as it's depends on o.

But then, I prefer it that way.

> > +/* 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?

INVALID is already for unknown message type. In this switch are listed
all the possible value that 'type' can get, as per the enum
libxl__qmp_message_type.

I'll add default: abort();

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

No I don't think so. QMP_COMMAND_NOT_FOUND can be useful when a qmp
command is removed (there is already one that is deprecated and we use).

The last two could be useful to user of libxl as they could provide
better error messages. xl don't care because whatever error message is
attach to the error, it will be printed.

Thanks,

-- 
Anthony PERARD

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