|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6.2 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*
On Thu, Nov 15, 2018 at 06:40:13PM +0000, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v6.2 05/11] libxl_qmp: Implementation of
> libxl__ev_qmp_*"):
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>
> Thanks for the additional comments. I got thoroughly stuck in.
>
> Overall the structure looks broadly right and my comments are
> generally about details. As might be expected, some of them are
> stylistic or matters of taste.
>
> Please feel free to push back if you disagree with me on anything.
>
> > struct libxl__ev_qmp {
> > /* caller should include this in their own struct */
> > /* caller must fill these in, and they must all remain valid */
> > libxl_domid domid;
> > libxl__ev_qmp_callback *callback;
> > int fd; /* set to send a fd with the command, -1 otherwise */
> > +
>
> This name `fd' becomes very confusing, because most of the time with
> qmp the fd in question is our socket to qemu. Can you rename it ?
> payload_fd maybe ?
payload_fd sound fine, I'll use that.
> > + /*
> > + * remaining fields are private to libxl_ev_qmp_*
> > + */
> > +
> > + int id;
> > + libxl__carefd *qmp_cfd;
> > + libxl__ev_fd qmp_efd;
> > + libxl__qmp_state qmp_state;
>
> What purpose do the qmp_ prefixes on these serve ? I think if it were
> me I would drop them and simply call these `cfd' and `efd' and `state'.
Sounds good.
>
> > +/* ------------ Implementation of libxl__ev_qmp ---------------- */
> > +
> > +/*
> > + * Possible internal state compared to qmp_state:
> > + *
> > + * qmp_state External qmp_cfd qmp_efd id rx_buf* tx_buf* msg*
> > + * disconnected Idle close Idle reset free free free
> ^^^^^
> YM `0' or `NULL'.
Yes.
> Maybe this table would be easier to read if the headings were offset
> from the values. Eg:
> + * qmp_state External qmp_cfd qmp_efd id rx_buf* tx_buf* msg*
> + * disconnected Idle close Idle reset free free free
> ?
> Up to you.
I'll give a try.
> > + * connecting Active open Active set used free set
>
> You say Active for the qmp_efd but I think it would be better to say
> what you are waiting for ? Looking at qmp_ev_connect I think
> connecting has only POLLIN. You could write IN, IN[|OUT], etc., maybe.
>
> While reading the rest of the code I found that this was often a
> critical piece of state you are managing, so I think some improvement
> here is warranted.
Indeed, it's important to not have POLLOUT all the time, or we just have
a busy loop.
> > + * capability_negotiation
> > + * Active open Active set used any set
>
> I would abbreviate this state name here, because it spoils the table.
> `cap.nego.' ? Or maybe you could just rename it to capab_nego or
> something.
Will do. 75cols is barely enough to write all the states :).
> > + * connected Connected open Active prev[1] used free any
>
> What msg might there be in state `connected' ? According to the
> public interface there is no message yet.
>
> In general the movement of the caller's intended qmp command from msg
> to rx_buf through to qemu could perhaps do with some exposition
> (commentary).
>
> > + * waiting_reply Active open Active set used any free
>
> I am a bit confused about the semantics of tx_buf being free in state
> waiting_reply. Does that mean the caller's wanted command has been
> sent ? This seems like part of the same question as I just asked...
>
> Maybe you want to split this into two rows:
>
> + * waiting_reply Active open IN|OUT set used user's free
> + * waiting_reply Active open IN set used free free
>
> ?
Spliting some states in two rows sound good, I might do it in other
cases as well.
> > + * broken[2] none[3] any Active any any any any
> ...
> > + * [1] id used on the previously sent command
>
> It would be better if the id column stated something more useful than
> `set'. `next' maybe, where applicable ?
When 'set', id is the id of the user command we intend to send or have
already sent and waiting for the associated reply. I'm not sure which
word or set of word would describe enough. I might need to add a comment
below the table.
> AIUI it is intended (important?) that ids should not be reused.
I think it's important, it can help detect mistakes. If we reuse the
same id, we might as well not have one.
I can also help to figure out if an error message is because a command
failed to execute or if the server itself had an error.
> > + * Possible buffers states:
> > + * - receiving buffer:
> > + * free used
> > + * rx_buf NULL allocated
> > + * rx_buf_size 0 allocation size of `rx_buf`
> > + * rx_buf_off 0 <= rx_buf_size
> > + * - transmiting buffer:
>
> Typo `transmitting'.
>
> A minor point, but indenting the rx_* things here would avoid them
> lining up with `transmitting buffer' and misleading the eye.
>
> Better still, have you considered moving this table into the struct
> itself ? You could put the table to the RHS of the actual member
> definitions. That gives slightly fewer places to look, although it
> would involve a cross-reference from this wider state description to
> the field's state tables.
Indeed, it feels kind of wrong to have the description of the internal
states in two different places. There are some descriptions in the
struct, but those are probably not needed.
> > + * free used
> > + * tx_buf NULL contain data
> > + * tx_buf_len 0 size of data
> > + * tx_buf_off 0 <= tx_buf_len
>
> You should explain the semantics of _off in both cases. It points
> into the buffer, but what is the meaning of the data (or the space)
> before and after it ?
>
> ... oh, I see for rx_buf_used this is documented in the struct
> itself. But not for tx_buf_off.
>
> Which leads me to say: the struct contains rx_buf_used but the comment
> here talks about _off.
:(, I need to fix that.
And here is the description:
rx_buf_used: actual data in the buffer
tx_buf_off: data already sent
> > + * - queued user command:
> > + * free set
> > + * msg NULL contain data
>
> YM `contains' data, in both cases.
>
> > + * msg_len 0 size of data
> > + *
> > + * - Allowed internal state transition:
> > + * disconnected -> connecting
> > + * connection -> capability_negotiation
> > + * capability_negotiation/waiting_reply -> connected
> > + * connected -> waiting_reply
> > + * any -> disconnected
>
> This does not mention the state `broken' and it should.
Maybe:
any -> broken
Or probably more accurate:
!disconnected -> broken
> > +static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
> > + /* !disconnected -> same state */
>
> This is not really true. This function is one to transform a
> transient, partial, internal state, into the corresponding proper
> internal state, by setting up the fd event.
>
> This will become more obvious if you add the IN etc. to the efd column
> in the internal state table.
Indeed, I'll fix the comment once the table have all the IN[|OUT].
> > + if (ev->tx_buf) {
> > + enable = true;
> > + } else {
> > + enable = (ev->qmp_state == qmp_state_connected) && ev->msg;
> > + }
> > +
> > + if (enable)
> > + events = ev->qmp_efd.events | POLLOUT;
> > + else
> > + events = ev->qmp_efd.events & ~POLLOUT;
> > +
> > + libxl__ev_fd_modify(gc, &ev->qmp_efd, events);
>
> This function seems more complicated than it needs to be ?
>
> AFAICT your code never clears POLLIN. And this function always
> unconditionally clears or sets POLLOUT. So the desired events do not
> in fact depend on qmp_efd.events ?
>
> Which makes sense, because from this name I would expect this function
> to figure out for itself exactly what events ought to be listened for.
>
> I think you want something like
> events = POLLIN;
> if (some stuff) {
> events |= POLLOUT;
> etc.
I'll fix that.
> > +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> > + libxl__qmp_state new_state)
> > +{
>
> Now that you have `broken' you should probably mention that this
> function is not used to enter the state `broken' (that is done
> willy-nilly), nor the state `disconnected' (that is done in _init).
>
> > +static int qmp_error_class_to_libxl_error_code(libxl__gc *gc,
> > + const char *eclass)
> > +{
> > + const libxl_enum_string_table *t = libxl_error_string_table;
> > +
> > + /* compare "QMP_GENERIC_ERROR" from libxl_error to "GenericError"
> > + * generated by the QMP server */
> > +
> > + for ( ; t->s; t++) {
> > + const char *s = eclass;
> > + const char *se = t->s;
> > + if (strncasecmp(t->s, "QMP_", 4))
> > + continue;
> > +
> > + /* skip "QMP_" */
> > + se += 4;
>
> This code contains or implies the value `4' a total of 4 times: three
> times in the code ("QMP_", 4, 4) and once in a comment. How about
> const char skip[] = "QMP_";
> const size_t skipl = sizeof(skip)-1;
> ?
Sounds good, I'll use that.
> > + }
> > +
> > + return ERROR_UNKNOWN_QMP_ERROR;
>
> I think you probably wanted to log the original error string here so
> that it does not become lost.
Right, I'll log it.
> > +static int qmp_ev_prepare_cmd(libxl__gc *gc,
> > + libxl__ev_qmp *ev,
> > + const char *cmd,
> > + const libxl__json_object *args)
> > + /* on entry: connecting/connected but with `msg` free
> > + * on return: same state but with `msg` set */
> > +{
> > + char *buf = NULL;
> > + size_t len;
> > +
> > + assert(!ev->tx_buf && !ev->tx_buf_len);
> > + assert(!ev->msg && !ev->msg_len);
> > +
> > + ev->id++;
> > + buf = qmp_prepare_cmd(gc, cmd, args, ev->id, &len);
> > + if (!buf) {
> > + return ERROR_FAIL;
> > + }
>
> There is no doc comment for qmp_prepare_cmd saying whether it logs on
> error. So it doesn't ? In which case maybe you should do so here.
>
> Alternatively if it can only really fail due to memory allocation
> failure, maybe it should return void (and call libxl__alloc_failed
> on allocation failure) instead ?
So we can have the allocation been done by libxl's functions, as
yajl_gen_alloc can use other functions that malloc/free/realloc, that
would remove the allocation failure.
But then I think that qmp_prepare_cmd could fail and return null if
'cmd' or 'args' contains bad json objects, or a wrong string.
So we still needs to detect failure.
I log here as I can add the domid.
> If you don't need to log I think you should drop the { } here.
>
> See also my comments on the other patch, about len.
I'll probably remove len and use strlen when needed.
> > +/* Setup connection */
> > +
> > +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> > + /* disconnected -> connecting but with `msg` free
> > + * on error: disconnected
> > + * If the initial state isn't disconnected, then nothing is done */
>
> Maybe it be better for this function to return `broken' on error ?
That not the case, but I could change to return broken on error, and let
the caller clean the state.
> Also, I think it is anomalous that this function handles `broken' as a
> no-op and claims success. I think that is not really what a caller
> might expect. Maybe you want to forbid `broken' (with a corresponding
> assert) or to permit it (calling dispose before doing the rest of the
> work).
I'm thinking to change the definition and only allow this function to be
called with the state disconnected, and have the caller check for the
current state.
> Contrary to the state description, this function does not transition
> rx_buf from free to used. However, I think this would probably be
> more easily remedied by changing the definition of `used' to permit
> NULL/0/0. You might want to use a different word to `used', `inuse'
> perhaps ?
Will do.
> > +out:
> > + libxl__carefd_close(ev->qmp_cfd);
> > + ev->qmp_cfd = NULL;
> > + return rc;
>
> If this function were permitted to leave the state `broken' on error,
> this separate error code would not be needed. Is there some reason
> not to do that ?
Not really.
> > +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> > + int fd, short events, short revents)
> > + /* On entry, ev_fd is (of course) Active. The ev_qmp may be in any
> > + * state where this is permitted. qmp_ev_fd_callback will do the work
> > + * necessary to make progress, depending on the current state, and make
> > + * the appropriate state transitions and callbacks. */
> > +{
> > + EGC_GC;
> > + int rc;
> > + libxl__json_object *o = NULL;
> > + libxl__ev_qmp *ev = CONTAINER_OF(ev_fd, *ev, qmp_efd);
> > +
> > + if (revents & (POLLHUP)) {
> > + LOGD(ERROR, ev->domid, "received POLLHUP from QMP socket");
> > + rc = ERROR_PROTOCOL_ERROR_QMP;
> > + goto out;
>
> This approach, without more, loses the error code from connect().
> I think you need to call getsockopt(fd, SOL_SOCKET, SO_ERROR, ...)
> and log the errno value if there is one.
>
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_10_16
I'll have a look.
> > + if (revents & POLLOUT) {
> > + rc = qmp_ev_callback_writable(gc, ev, fd);
> ...
> > + if (revents & POLLIN) {
> > + rc = qmp_ev_callback_readable(egc, ev, fd);
> > + if (rc)
> > + goto out;
> > +
> > + /* parse input */
>
> I find it odd that this input parsing is not part of
> qmp_ev_callback_readable. What do you think about moving it there ?
I wanted to reduce the number of function calls between when a user
callback is called and when ev_qmp's control passes outsite of ev_qmp's
implementation. So I moved qmp_ev_handle_message() call here.
> > +static int qmp_ev_callback_writable(libxl__gc *gc,
> > + libxl__ev_qmp *ev, int fd)
> > + /* on entry: !disconnected
> > + * on return, one of these state transition:
> > + * connected -> waiting_reply
> > + * * -> state unchange
> > + * but with the state of tx_buf changed
> > + * on error: broken */
> > +{
> > + int rc;
> > + ssize_t r;
> > +
> > + if (ev->qmp_state == qmp_state_connected) {
> > + assert(!ev->tx_buf);
> > + if (ev->msg) {
>
> See my comments about the state `connected' and msg.
>
> > + /*
> > + * We will send a file descriptor associated with a command on the
> > + * first byte of this command.
> > + */
> > + if (ev->qmp_state == qmp_state_waiting_reply &&
> > + ev->fd >= 0 &&
> > + ev->tx_buf_off == 0) {
>
> According to the doc comments, state waiting_reply might have tx_buf
> free, in which case it would try to execute this code. I don't think
> that can be right.
When tx_buf is free, that function returns earlier. So this code isn't
executed.
> > + if (ev->tx_buf_off == ev->tx_buf_len) {
> > + free(ev->tx_buf);
> > + ev->tx_buf = NULL;
> > + ev->tx_buf_len = ev->tx_buf_off = 0;
>
> This appears in _init too. Maybe you want a qmp_ev_tx_buf_clear
> function ?
Sounds good.
> > +static int qmp_ev_callback_readable(libxl__egc *egc,
> > + libxl__ev_qmp *ev, int fd)
> > + /* !disconnected -> same state (with rx buffer updated)
> > + * on error -> broken */
> > +{
> > + EGC_GC;
> > +
> > + while (1) {
> > + ssize_t r;
> > +
> > + /* Check if the buffer still have space, or increase size */
> > + if (ev->rx_buf_size - ev->rx_buf_used < QMP_RECEIVE_BUFFER_SIZE) {
> > + ev->rx_buf_size = max(ev->rx_buf_size * 2,
> > + (size_t)QMP_RECEIVE_BUFFER_SIZE * 2);
>
> How about
>
> + ev->rx_buf_size *= 2;
> + ev->rx_buf_size += QMP_RECEIVE_BUFFER_SIZE;
>
> or some such ? That's easier to read than a conditional.
That looks fine.
> > + assert(ev->rx_buf_size <= QMP_MAX_SIZE_RX_BUF);
> > + if (ev->rx_buf_size > QMP_MAX_SIZE_RX_BUF) {
>
> I think the assert is redundant.
>
> > + LOGD(ERROR, ev->domid,
> > + "QMP receive buffer is too big (%ld > %lld)",
> > + ev->rx_buf_size, QMP_MAX_SIZE_RX_BUF);
> > + return ERROR_BUFFERFULL;
> > + }
>
> Putting this error check between the change to rx_buf_size and the
> change to rx_buf obscures the essential connection between both
> changes, which must be indivisible.
>
> It also results in a bug: if this error trips, rx_buf_size has been
> updated but rx_buf has not. This is a dangerously invalid state.
>
> This function tries to read everything that qemu produces and calls
> the whole thing a loss of qemu produces more than QMP_MAX_SIZE_RX_BUF.
> But perhaps qemu producing more than QMP_MAX_SIZE_RX_BUF output might
> not be the result of qemu going mad. It might be the result of us
> having ignored it for a while, and qemu filling a buffer with event
> notifications which we might want to discard ?
>
> Would it not be better to process messages as they arrive ? Ie to put
> the attempt to find valid messages here inside this loop ?
Maybe.
We don't even needs to wait for a valid message and start parsing. yajl
allow to parse partial json input. But I don't know if it is a good idea
to start doing that.
But in anycase, it is probably better to look for valide messages before
returning an error with "there's too much to process".
> That might also allow the reduction of the maximum message size from
> 8Mby. 8Mby seems like quite a lot.
I don't know how big a message can be. I can certainly try to start QEMU
with 256 cpus and check the size of query-cpu's response.
> Of such a change would result in a possible return to the caller with
> the qmp fd having pending data to read. We would have to make sure to
> actually call read again after sending a new command, in case the efd
> is edge triggered.
>
> > +/* Handle messages received from QMP server */
> > +
> > +static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
> > + libxl__json_object **o_r)
> > + /* Find a JSON object and store it in o_r.
> > + * return ERROR_NOTFOUND if no object is found.
> > + * `o_r` is allocated within `egc`.
> > + *
> > + * !disconnected -> same state (with rx buffer updated)
> > + */
> > +{
> ...
> > + /* Search for the end of a QMP message: "\r\n" */
> > + end = memmem(ev->rx_buf, ev->rx_buf_used, "\r\n", 2);
> > + if (!end)
> > + return ERROR_NOTFOUND;
> > + len = (end - ev->rx_buf) + 2;
>
> In:
>
> Subject: Re: [PATCH v5 03/15] libxl_qmp: Implement fd callback and read
> data [and 1 more messages]
> Date: Mon, 29 Oct 2018 17:31:59 +0000
>
> I wrote:
>
> But I think you should treat only `\n' as the delimiter. This will
> considerably simplify the buffer handling. (You should check and trim
> the preceding `\r' before passing things to libxl__json_parse of
> course.)
>
> But you don't seem to have done that, or replied ?
This is still simpler than what we had before, and if I look for only
'\n', I would need a loop in cases where \n isn't precedded by \r. I
also check the whole buffer rather than new data, so looking for \r and
check if it is followed by a \n is similar.
As for triming the \r, this is done before the libxl__json_parse call.
> > +static int qmp_ev_handle_message(libxl__egc *egc,
> > + libxl__ev_qmp *ev,
> > + const libxl__json_object *resp)
> > + /*
> > + * This function will handle every messages sent by the QMP server.
> > + * Return values:
> > + * < 0 libxl error code
> > + * 0 success
> > + * 1 success, but a user callback has been called,
> > + * `ev` should not be used anymore.
> ...
> > + case LIBXL__QMP_MESSAGE_TYPE_QMP:
> ...
> > + assert(!ev->tx_buf);
> > + ev->tx_buf = qmp_prepare_cmd(gc, "qmp_capabilities", NULL,
> > + QMP_CAPABILITY_NEGOTIATION_MSGID,
> > + &ev->tx_buf_len);
> > + ev->tx_buf_off = 0;
>
> Why do you not use qmp_ev_prepare_cmd for this ?
Because qmp_ev_prepare_cmd does more like increasing the id. This
qmp_capabilities is a special command that is part of the
initialisation. I need to know which id is used so that I can track it
later and found out when QEMU is ready to receive other commands.
We could call qmp_ev_prepare_cmd, qmp_ev_prepare_user_cmd.
> > + case LIBXL__QMP_MESSAGE_TYPE_RETURN:
> > + case LIBXL__QMP_MESSAGE_TYPE_ERROR:
> ...
> > + o = libxl__json_map_get("id", resp, JSON_INTEGER);
> > + if (!o) {
> > + /*
> > + * If "id" isn't present, an error occur on the server before
> > + * it has read the "id" provided by libxl.
> > + */
> > + qmp_ev_parse_error_messages(egc, ev, resp);
> > + return ERROR_PROTOCOL_ERROR_QMP;
>
> I think you wanted to return the error code from
> qmp_ev_parse_error_messages ?
Yeah, but then I've added to the public documentation that ERROR_QMP_*
means that ev_qmp is still Connected.
But here, if 'id' isn't in the message, something has gone wrong and I
don't think there is any point trying to continue. This isn't supposed
to happen when everything else has been fine from libxl's point of view.
The error code returned here will be passed to the user's callback.
> > + id = libxl__json_object_get_integer(o);
> > +
> > + if (id == QMP_CAPABILITY_NEGOTIATION_MSGID) {
> > + /* We have a response to our qmp_capabilities cmd */
> > + if (ev->qmp_state != qmp_state_capability_negotiation ||
> > + type != LIBXL__QMP_MESSAGE_TYPE_RETURN)
> > + goto out_unknown_id;
> > + qmp_ev_set_state(gc, ev, qmp_state_connected);
> > + return 0;
> > + }
>
> I'm a bit puzzled about the capability negotation, its id, etc.:
>
> * Why does QMP_CAPABILITY_NEGOTIATION_MSGID exist at all ? You
> could just use a normal id, couldn't you ? And you'd be able to
> tell from your own state that it was the right value.
But I would then needs to track two different ids, one for the
qmp_capabilities command, and one for the user's command.
> * Why are we even doing capability negotation when we throw the answer
> away ?
It is part of the protocol.
And qmp_capabilities doesn't return anything (just an empty object).
> * I think it would be better to check id == ev->id first, rather
> than in each of these branches.
I don't think I follow. id==ev->id is only checked once.
> > + if (ev->qmp_state == qmp_state_waiting_reply &&
> > + id == ev->id) {
> > + if (type == LIBXL__QMP_MESSAGE_TYPE_RETURN) {
> > + response = libxl__json_map_get("return", resp, JSON_ANY);
> > + rc = 0;
> > + } else {
> > + /* error message */
> > + response = NULL;
> > + rc = qmp_ev_parse_error_messages(egc, ev, resp);
> > + }
> > + qmp_ev_set_state(gc, ev, qmp_state_connected);
> > + ev->callback(egc, ev, response, rc); /* must be last */
> > + return 1;
> > + }
> > +
> > +out_unknown_id:
>
> We're just falling through here. It seems weird to me.
>
> I would expect this sequence of checks:
>
> 1. Check id against our id; should be the same; if not, this
> is an out of sequence reply which is not possible, so
> call it a protocol error.
>
> 2. Check if the thing is an error response; if it is, parse and
> return the error message.
>
> 2. It must be a RETURN. (Maybe add an assert here.) Check our
> state, which should be capability_negotiation (in which case we
> go to the next thing) or waiting_reply (in which case we're
> done). Otherwise it is a protocol error.
I don't think all ERROR should be protocol error. To me, it is fine if a
command returns an error, then tell the user about it. Also, there is no
needs to tear down the connection in this case.
As for the sequence of checks, I can try to follow it, it might look
weird because there is two differents ids to check, which id is valide
depends on the current state.
> > +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> > + const char *cmd, libxl__json_object *args)
> > + /* disconnected -> connecting
> > + * connected -> waiting_reply
> > + * on error: disconnected */
> > +{
> > + int rc;
> > +
> > + LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
> > +
> > + assert(ev->qmp_state == qmp_state_disconnected ||
> > + ev->qmp_state == qmp_state_connected);
> > +
> > + /* Connect to QEMU if not already connected */
> > + rc = qmp_ev_connect(gc, ev);
> > + if (rc)
> > + goto out;
> > +
> > + rc = qmp_ev_prepare_cmd(gc, ev, cmd, args);
> > + if (rc)
> > + goto out;
> > +
> > + qmp_ev_ensure_reading_writing(gc, ev);
> > +
> > +out:
>
> Please add
> rc = 0;
> before out;
>
> But you might prefer just to
> return 0;
> since there is nothing allocated in this function that ought to be
> disposed (which is nice).
Will do.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |