|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*
Anthony PERARD writes ("[PATCH v7 06/14] libxl_qmp: Implementation of
libxl__ev_qmp_*"):
> This patch implement the API libxl__ev_qmp documented in the previous
> patch, "libxl: Design of an async API to issue QMP commands to QEMU".
>
> Since this API is to interact with QEMU via the QMP protocol, it also
> implement a QMP client. The specification for the QEMU Machine Protocol
> (QMP) can be found in the QEMU repository at:
> https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/interop/qmp-spec.txt
Thanks.
I have only fairly minor comments now. The biggest one remaining is
about the use of EGC_GC which I think probably wants to become
STATE_AO_GC throughout. See below...
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 1c7a3b22f4..056de9de2f 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -412,6 +412,19 @@ _hidden int libxl__ev_qmp_send(libxl__gc *gc,
> libxl__ev_qmp *ev,
> + /* receive buffer */
> + char *rx_buf;
rx_buf needs a comment saying it comes from NOGC since otherwise one
would assume it came from the ao gc like the other buffers.
(Could it come from the ao gc instead?)
> +static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
> + /* Update the state of `efd` to match the permited state */
> +{
This function is legal only in states other than disconnected.
Needs to be documented.
> +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> + libxl__qmp_state new_state)
> + /* on entry: !broken and !disconnected */
> +{
> + switch (new_state) {
> + case qmp_state_disconnected:
> + break;
> + case qmp_state_connecting:
> + assert(ev->state == qmp_state_disconnected);
> + break;
> + case qmp_state_capability_negotiation:
> + assert(ev->state == qmp_state_connecting);
> + break;
> + case qmp_state_waiting_reply:
> + assert(ev->state == qmp_state_capability_negotiation ||
> + ev->state == qmp_state_connected);
> + break;
> + case qmp_state_connected:
> + assert(ev->state == qmp_state_waiting_reply);
> + break;
> + }
> +
> + ev->state = new_state;
I think this function needs to update efd ? What am I missing ?
The comment doesn't say what the output state is but naturally I
assume that it is precisely new_state, and not some transitional
mixture. If it is intended to produce a transitional mixture that
ought to be documented.
For a concrete example: if on entry, with new_state==disconnected, we
are `connecting' then: efd will be looking for POLLIN. But it needs
to become Idle.
> +/* Setup connection */
> +
> +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> + /* disconnected -> connecting but with `msg` free
> + * on error: broken */
> +{
This function looks fine to me. However, earlier I wrote this:
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 ?
This is still true. That is, your state description for `connecting'
says that rx_buf is `used'. And your description lower about what
rx_buf being `used' means says that rx_buf must be `allocated'.
I think this would probably be best resolved by writing:
* free used
- * rx_buf NULL allocated
+ * rx_buf NULL NULL or allocated
* rx_buf_size 0 allocation size of `rx_buf`
* rx_buf_used 0 <= rx_buf_size, actual data in the
* buffer
Ie just to change the internal spec. I am going to assume for the
rest of the review that the code is right and the internal spec will
be updated. (I don't think it is necessary to change the descriptions
of rx_buf_size and rx_buf_used; it will be clear that the `allocation
size' of a NULL must be 0.)
> +/* QMP FD callbacks */
> +
> +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
...
> +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:
> + * waiting_reply (with msg set) -> waiting_reply (with msg free)
> + * tx_buf set -> same state or tx_buf free
> + * tx_buf free -> no state change
> + * on error: broken */
> +{
...
> + assert(ev->tx_buf);
> + if (!ev->tx_buf)
> + return 0;
I think the if is vestigial.
> + while (ev->tx_buf_off < ev->tx_buf_len) {
> + r = write(fd, ev->tx_buf + ev->tx_buf_off,
> + ev->tx_buf_len - ev->tx_buf_off);
> + if (r < 0) {
> + if (errno == EINTR)
> + continue;
> + if (errno == EWOULDBLOCK)
> + break;
> + LOGED(ERROR, ev->domid, "failed to write to QMP socket");
> + return ERROR_FAIL;
> + }
> + ev->tx_buf_off += r;
Can you assert that the value of r was within range ? (Perhaps this
is paranoia on my part, but, still.)
> +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`.
Why not allocate o_r from within AO_GC ?
ISTM that taking it from egc is a beartrap. If you do want to
allocate it from egc, this should definitely be documented in the
internal public api for libxl__ev_qmp_callback. Right now a caller
might well reasonably assume that the libxl__json_object *response
they get is useable for the whole ao. Indeed future callers might
even need that semantic.
To do this, use STATE_AO_GC instead of EGC_GC.
TBH I think you should probably do that throughout.
> +static int qmp_ev_parse_error_messages(libxl__egc *egc,
> + libxl__ev_qmp *ev,
> + const libxl__json_object *resp)
Personally I would have preferred to read this and
qmp_ev_handle_message in the other order, so I would swap them. Up to
you.
> + /* no state change */
> +{
> + EGC_GC;
> + int rc;
> + const char *s;
> + const libxl__json_object *o;
> + const libxl__json_object *err;
> +
> + /*
> + * { "error": { "class": string, "desc": string } }
> + */
> +
> + err = libxl__json_map_get("error", resp, JSON_MAP);
> +
> + o = libxl__json_map_get("class", err, JSON_STRING);
I wondered: surely err can be NULL ? I didn't find any docs saying
that it couldn't; nor that it tolerated NULL for o on input.
However, reading the implementation I see that libxl__json_map_get
calls libxl__json_object_is_map which does indeed handle o==0.
Could you perhaps add a comment (in libxl_internal.h near
libxl__json_map_get) et al about this ?
> +static int qmp_ev_handle_message(libxl__egc *egc,
> + libxl__ev_qmp *ev,
> + const libxl__json_object *resp)
...
> + /* Prepare next message to send */
> + assert(!ev->tx_buf);
> + ev->id = ev->next_id++;
> + buf = qmp_prepare_cmd(libxl__ao_inprogress_gc(ev->ao),
> + "qmp_capabilities", NULL, ev->id);
Erk, I don't like the open coded call to libxl__ao_inprogress_gc. I'm
becoming more convinced you just wanted AO_GC or STATE_AO_GC
everywhere.
> +void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
> + /* * -> disconnected */
> +{
> + LOGD(DEBUG, ev->domid, " ev %p", ev);
> +
> + free(ev->rx_buf);
> +
> + libxl__ev_fd_deregister(gc, &ev->efd);
> + libxl__carefd_close(ev->cfd);
> +
> + libxl__ev_qmp_init(ev);
> +}
It's a small point, but it would be nicer to move the free of rx_buf
nearer the call to libxl__ev_qmp_init which zeroes it.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |