Re: [PATCH v7 06/14] libxl_qmp: Implementation of libxl__ev_qmp_*

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

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel