Re: [Xen-devel] [PATCH v4 23/32] libxl_qmp: Respond to QMP greeting

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 02, 2018 at 01:26:35PM +0200, Roger Pau Monné wrote:
> On Fri, Jul 27, 2018 at 03:06:05PM +0100, Anthony PERARD wrote:
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > ---
> >  tools/libxl/libxl_qmp.c | 36 ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > index e649b8054d..83afce3192 100644
> > --- a/tools/libxl/libxl_qmp.c
> > +++ b/tools/libxl/libxl_qmp.c
> > @@ -1309,6 +1309,9 @@ int libxl__qmp_initializations(libxl__gc *gc, 
> > uint32_t domid,
> >  
> >  /* ------------ Implementation of libxl__ev_qmp ---------------- */
> >  
> > +/* hard coded message ID used for capability negociation */
> > +static int qmp_capability_negociation_msgid = 1;
> 
> Could this be a define instead? Or at least constify it because it
> doesn't seem like it's modified (at least in this patch).

I guess a define will do.

> >  static int qmp_ev_prepare_cmd(libxl__gc *gc,
> >                                libxl__ev_qmp *ev,
> >                                const char *cmd,
> > @@ -1379,7 +1382,7 @@ static int qmp_ev_handle_response(libxl__egc *egc,
> >      }
> >  
> >      id = libxl__json_object_get_integer(o);
> > -    if (id != ev->id)
> > +    if (id != ev->id && id != qmp_capability_negociation_msgid)
> >          return 0;
> >  
> >      switch (type) {
> > @@ -1413,9 +1416,21 @@ static int qmp_ev_handle_response(libxl__egc *egc,
> >          abort();
> >      }
> >  
> > -    ev->id = -1;
> > -    ev->callback(egc, ev, response, rc); /* must be last */
> > -    return 1;
> > +    /*
> > +     * Even if the current state is capability_negociation and the correct 
> > ID
> > +     * as been received, call the callback on error.
> > +     */
> > +    if (ev->qmp_state == qmp_state_capability_negociation &&
> > +        id == qmp_capability_negociation_msgid &&
> > +        rc == 0) {
> > +        ev->qmp_state = qmp_state_connected;
> > +        libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | 
> > POLLOUT);
> > +        return 0;
> > +    } else {
> > +        ev->id = -1;
> > +        ev->callback(egc, ev, response, rc); /* must be last */
> > +        return 1;
> > +    }
> >  }
> >  
> >  /* return 1 when a user callback as been called */
> > @@ -1429,6 +1444,11 @@ static int qmp_ev_handle_message(libxl__egc *egc,
> >      switch (type) {
> >      case LIBXL__QMP_MESSAGE_TYPE_QMP:
> >          /* greeting message */
> > +        assert(ev->qmp_state == qmp_state_connecting);
> > +        ev->qmp_state = qmp_state_greeting;
> > +        /* Allow qmp_ev_callback_writable to be called in order to send
> > +         * qmp_capabilities */
> > +        libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events | 
> > POLLOUT);
> >          return 0;
> >      case LIBXL__QMP_MESSAGE_TYPE_RETURN:
> >      case LIBXL__QMP_MESSAGE_TYPE_ERROR:
> > @@ -1563,7 +1583,11 @@ static int qmp_ev_callback_writable(libxl__gc *gc, 
> > libxl__ev_qmp *ev, int fd)
> >       * be send for now will be in this call. */
> >      libxl__ev_fd_modify(gc, &ev->qmp_efd, ev->qmp_efd.events & ~POLLOUT);
> >  
> > -    if (ev->qmp_state == qmp_state_connected) {
> > +    if (ev->qmp_state == qmp_state_greeting) {
> > +        buf = qmp_prepare_qmp_cmd(gc, "qmp_capabilities", NULL,
> > +                                  qmp_capability_negociation_msgid, &len);
> > +        ev->qmp_state = qmp_state_capability_negociation;
> > +    } else if (ev->qmp_state == qmp_state_connected) {
> 
> A switch (ev->qmp_state) might be better.

I guess, it's less repetition of ev->qmp_state. I'll switch.

Thanks,

-- 
Anthony PERARD

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