|
[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_*
On Fri, Dec 21, 2018 at 03:36:18PM +0000, Ian Jackson wrote:
> 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?)
I guess it's fine to have the rx_buf comes from ao gc. The buffer isn't
that big (maybe 100kB in worse cases).
>
> > +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.
Will do.
> > +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 ?
Yes, I think it's fine to call qmp_ev_ensure_reading_writing here ( or
even inline it) and qmp_ev_ensure_reading_writing won't needs to be
call from other places.
> 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.
Once I update efd with this function, I think qmp_ev_set_state should
always output precisely new_state. But new_state alone might not be
enough in few cases (waiting_reply) to describe a full state, but it
will be one of the possible internal state as describe in the state
documentation of the implementation.
>
> > +/* 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
I'll make this change.
> 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.
I'll remove it.
> > + 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.)
I do assert the value of tx_buf_off which does include r.
But I can add
`assert(r > 0 && r <= (ev->rx_buf_size - ev->rx_buf_used));'.
> > +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.
Indeed, that can be an issue. I'll make the changes to use ao gc instead
of egc.
> 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)
> > + /* 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 ?
I'll add the comments.
> > +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.
As rx_buf is probably going to be allocated from ao gc, the free won't
be needed anymore. Or, I could realloc with a new size of 0:
libxl__realloc(maybe_gc, ev->rx_buf, 0);
and that would free the memory. I've check realloc, libxl__realloc and
libxl__free_all, and they all seems to do the right things when the new
size is 0.
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 |