Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read data [and 1 more messages]

[Anthony PERARD <anthony.perard@citrix.com>]

On Mon, Oct 15, 2018 at 05:35:36PM +0100, Ian Jackson wrote:
> > +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_DEVICENOTFOUND:
> > +        return ERROR_QMP_DEVICE_NOT_FOUND;
> 
> Urgh.  The slightly different names means that this can't be
> macro-ified.  Without that, it would be really easy for someone in the
> future to accidentally write something like this:
>
> > +    case LIBXL__QMP_ERROR_CLASS_GENERICERROR:
> > +        return ERROR_QMP_GENERIC_ERROR;
> > +    case LIBXL__QMP_ERROR_CLASS_COMMANDNOTFOUND:
> > +        return ERROR_QMP_GENERIC_ERROR;
> > +    case LIBXL__QMP_ERROR_CLASS_DEVICENOTFOUND:
> > +        return ERROR_QMP_DEVICE_NOT_FOUND;
> 
> and it's hard to spot.
> 
> What are LIBXL__QMP_ERROR_CLASSes and why are they even different from
> ERROR_* values ?  Maybe one of them is the QMP integer values and one
> of them is the corresponding libxl integer values ?

The issue here is that, in QMP, the error is a string and in camel case,
e.g. "GenericError".  I've use the idl to generate an enum from the
different error strings.

> Anyway, can we not make this less open-coded somehow.

I could remove some underscores '_' from the libxl ERROR_* integer
values, e.g. ERROR_QMP_GENERICERROR. But if you have a better suggestion
on how to transform "CommandNotFound" into a ERROR_QMP_*, I'll take it.

> > +    default:
> > +        abort();
> 
> But what happens if qemu invents a new error code ?  I don't think
> aborting is likely to be right.

If qemu invent a new error code, libxl should not call
qmp_error_class_to_libxl_error_code(), because libxl would not have been
able to generate a libxl__qmp_error_class from the new error code.

But I could add an assert(false) and return ERROR_FAIL instead of the
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);
> 
> What is the dead store from "error" doing ?

It's not dead, I reuse "o", as I get "desc" from "o".
I could just create new variables here instead of reusing the same one
(o) over and over again, e.g. obj_id, obj_error, obj_desc.

Should I add here that we are parsing: {"error":{"desc": X }} ?

> > +    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);
> > +        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;
> 
> Surely this should print more debugging (or maybe even error log
> messages) if the error json document was not in the expected form ?

Do you mean that if the QMP server doesn't respect the QMP protocol, I
should print more debug here?

If we are at this point in the programme, we know that the server sent:
{"error": X }

I'm tempted to say that if "class" or "desc" isn't present, we could say
so and declare the server broken (and disconnect).

> > +        /* Findout how much can be parsed */
> > +        size_t len = end - s;
> > +
> > +        LOG_QMP("parsing %luB: '%.*s'", len, (int)len, s);
> > +
> > +        /* Replace \n by \0 so that libxl__json_parse can use strlen */
> > +        s[len - 1] = '\0';
> 
> Doesn't this feed the \r to libxl__json_parse ?
> 
> Also, why does this have to be a loop ?  Does qemu really send
> multiple json documents end to end, and only sometimes with
> intervening \r\n ?  Does it ever send a json document without \r\n at
> the end and then stop speaking for a while ?

QEMU will send two messages at once, e.g. when asking to eject or change
the cdrom, QEMU will often send the response to our command and an event
at the same time. So we need a loop to parse all messages received.

But there is always a \r\n at the end of every commands.

> > +        ev->buf_consumed += len;
> > +
> > +        if (ev->buf_consumed >= ev->buf_used) {
> > +            free(ev->rx_buf);
> 
> Surely buf_consumed <= buf_used ?  Maybe you should assert that.

I'm attempting to detect here if there's something left in the buffer
that hasn't been parsed yet. If there is nothing left, we can free the
buffer.

Or do you mean that the condition should be buf_consumed == buf_used ?

> > +static int qmp_ev_callback_writable(libxl__gc *gc, libxl__ev_qmp *ev, int fd)
> > +{
> > +    int rc;
> > +    char *buf;
> > +    size_t len;
> > +    int send_fd = -1;
> > +
> > +    /* No need to call qmp_ev_callback_writable again, everything that needs to
> > +     * be send for now will be in this call. */
> 
> So you guarantee never to send any message which is too large to fit
> into a socket buffer ?  Do you know what that length is ?

I'm using libxl_write_exactly().

On the other end, I've just realize that I'm also using
libxl__sendmsg_fds(), which I guess may not send everything. But the
function also doesn't seems to the caller know how much have been sent.

Should I keep using libxl_write_exactly() ?
I'm going to need to be able to track how much data have been sent for
at least sendmsg() anyway, and try again later.

> > +static void qmp_ev_callback_error(libxl__egc *egc, libxl__ev_qmp *ev)
> > +{
> > +    EGC_GC;
> > +
> > +    LOGD(ERROR, ev->domid, "Error happend with the QMP connection to QEMU");
> > +
> > +    /* On error, deallocate all private ressources */
> > +    libxl__ev_qmp_dispose(gc, ev);
> 
> This surely needs to set the state.  Presumably that should be done in
> libxl__ev_qmp_dispose but AFAICT it isn't.

It is done by libxl__ev_qmp_init, which _dispose calls, itn't that
enough?

> > +void libxl__ev_qmp_init(libxl__ev_qmp *ev)
> > +{
> > +    ev->id = -1;
> > +
> > +    ev->qmp_cfd = NULL;
> > +    libxl__ev_fd_init(&ev->qmp_efd);
> > +    ev->qmp_state = qmp_state_disconnected;
> > +    ev->last_id_used = QMP_CAPABILITY_NEGOTIATION_MSGID + 1;
> 
> Going back a bit, earlier we had:
> 
> > +#define QMP_CAPABILITY_NEGOTIATION_MSGID 1
> 
> I recommend using a different value.  Can we safely let this wrap ?
> If so maybe we could use 0x786c7100 which spells "xlq\0" or something.
> This can make it easier to see where rogue values are coming from, to
> distinguish arguments in debug output, etc.

ID can be string, or any json value. Maybe we could have
{"libxl-id": number} as "id"?
But 0x786c7100 as starting point would work fine as well.
I don't think it matter if the value wrap. Beside adding 1 to generate a
new id, and compare, I don't think the id should be use for anything
else.

Thanks,

-- 
Anthony PERARD

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