Re: [PATCH v5 02/15] libxl_qmp: Connect to QMP socket

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

On Wed, Oct 10, 2018 at 04:29:03PM +0100, Ian Jackson wrote:
> Anthony PERARD writes ("[PATCH v5 02/15] libxl_qmp: Connect to QMP socket"):
> > This is a first patch to implement libxl__ev_qmp, it only connects to
> > the QMP socket of QEMU and registers a fd callback that does nothing.
> ...
> > +typedef enum {
> > +    /* initial state */
> > +    qmp_state_disconnected = 1,
> > +    /* connected to QMP socket, waiting for greeting message */
> > +    qmp_state_connecting,
> > +    /* greeting message received */
> > +    qmp_state_greeting,
> > +    /* qmp_capabilities command sent, waiting for reply */
> > +    qmp_state_capability_negotiation,
> > +    /* ready to send commands */
> > +    qmp_state_connected,
> > +} libxl__qmp_state;
> 
> IWBN to relate these private states to the outward-facing API states
> like `Connected'.

I think, that would be:
  qmp_state_disconnected,             Idle
  qmp_state_connecting,               Active
  qmp_state_greeting,                 Active
  qmp_state_capability_negotiation,   Active
  qmp_state_connected,                Active/Connected

I can try to squeeze this information somewhere.

> I often write a table of legal field values - see for example,
> libxl_exec.c near l.241 and libxl_stream_read.c near l.35.  But maybe
> this is not complicated enough to need that.

Indeed, I don't think the relation is complicated enough.

> > +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> > +{
> ...
> > +    r = connect(libxl__carefd_fd(ev->qmp_cfd),
> > +                (struct sockaddr *) &un, sizeof(un));
> 
> Up to here:
> 
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> But:
> 
> > +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> > +                       const char *cmd, libxl__json_object *args)
> > +{
> > +    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
> > +
> > +    /* Connect to QEMU if not already connected */
> > +    return qmp_ev_connect(gc, ev);
> > +}
> 
> I think it would be nicer to read a complete implementation of this
> function.  Right now it's obviously wrong and impossible to review.
> 
> So please postpone this hunk.

It would have been nice if I could, but we can't have unused static
function. But let me reply to the review of the next patch.

-- 
Anthony PERARD

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