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

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

(Skipping the parts where you were just agreeing with me...)

Anthony PERARD writes ("Re: [PATCH v7 06/14] libxl_qmp: Implementation of 
libxl__ev_qmp_*"):
> On Fri, Dec 21, 2018 at 03:36:18PM +0000, Ian Jackson wrote:
> > > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > > +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> > > +                             libxl__qmp_state new_state)
> > > +    /* on entry: !broken and !disconnected */
> > > +{
...
> > 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.

Sure.

> > 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.

That SGTM.

> > > +    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));'.

Yes.  As I say worrying that write() might return something insane is
perhaps paranoia.

Do you mean `ev->tx_buf_len - ev->tx_buf_off' like in the write call ?
I would factor the expression out into a variable so you only have to
write it once.

> > 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.

Thanks.

> > > +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.

Yay, less code.

> 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.

We can probably afford to leak it for the duration of the ao.

Ian.

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