Re: [Xen-devel] [PATCH v4 13/32] libxl_qmp: Connect to QMP socket

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Aug 02, 2018 at 11:35:53AM +0200, Roger Pau Monné wrote:
> On Fri, Jul 27, 2018 at 03:05:55PM +0100, Anthony PERARD wrote:
> > +typedef enum {
> > +    qmp_state_disconnected = 1,
> > +    qmp_state_connecting,
> > +    qmp_state_greeting,
> > +    qmp_state_capability_negociation,
> > +    qmp_state_connected,
> > +} libxl__qmp_state;
> > +
> 
> I think this should be declared in libxl_types_internal.idl?

I don't know, I kind of wanted the enum to be contained in libxl_qmp.c,
but then I couldn't use the enum type in the struct here.

Also the idl provides more than needed (conversion to string) and make
the names more verbose, by adding libxl__ prefix.

> And (maybe) a description of each state would be helpful for future
> reference?

Yes, but not in libxl_internal.h, the description should be in
libxl_qmp.c as this qmp_state is not part of the API.

> >  struct libxl__ev_qmp {
> >      /* caller should include this in their own struct */
> >      /* caller must fill these in, and they must all remain valid */
> > @@ -427,6 +435,9 @@ struct libxl__ev_qmp {
> >      /* remaining fields are private to libxl_ev_qmp_* */
> >  
> >      int id;
> > +    libxl__carefd *qmp_cfd;
> > +    libxl__ev_fd qmp_efd;
> > +    libxl__qmp_state qmp_state;
> >  };
> >  
> >  
> > diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
> > index c5e05e5679..96a347dd3b 100644
> > --- a/tools/libxl/libxl_qmp.c
> > +++ b/tools/libxl/libxl_qmp.c
> > +out:
> > +    libxl__carefd_close(ev->qmp_cfd);
> > +    ev->qmp_cfd = NULL;
> > +    return rc;
> > +}
> > +
> > +
> 
> Double newline.

I like space, it gives some separations before the next big chunk of
code.

> > +/*
> > + * libxl__ev_qmp_*
> > + */
> > +
> > +void libxl__ev_qmp_init(libxl__ev_qmp *ev)
> > +{
> > +    ev->id = -1;
> > +
> > +    ev->qmp_cfd = NULL;
> > +    libxl__ev_fd_init(&ev->qmp_efd);
> > +    ev->qmp_state = qmp_state_disconnected;
> > +}
> > +
> > +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> > +                       const char *cmd, libxl__json_object *args)
> > +{
> > +    int rc;
> > +
> > +    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
> > +
> > +    /* Connect to QEMU if not already connected */
> > +    rc = qmp_ev_connect(gc, ev);
> > +
> > +    return rc;
> 
> You can get rid of rc and just use:
> 
> return qmp_ev_connect(gc, ev);

Will do. (And introduce the rc in the following patches.)

> > +}
> > +
> > +void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev)
> > +{
> > +    LOGD(DEBUG, ev->domid, " ev %p", ev);
> > +
> > +    libxl__ev_fd_deregister(gc, &ev->qmp_efd);
> > +    libxl__carefd_close(ev->qmp_cfd);
> > +    ev->qmp_cfd = NULL;
> 
> No need to set qmp_cfd to NULL if you call _init afterwards.

Will not do then.

> > +
> > +    libxl__ev_qmp_init(ev);
> 
> Thanks, Roger.

Thanks Roger,

-- 
Anthony PERARD

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