[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v5 02/15] libxl_qmp: Connect to QMP socket
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@xxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |