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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 27, 2018 at 03:05:55PM +0100, Anthony PERARD wrote:
> This is a first patch to implement libxl__ev_qmp, it only connect to the
                                                            ^ connects
> QMP socket of QEMU and register a fd callback that does nothing.
                         ^ registers
> 
> Callback functions will be implemented in following patches.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
>  tools/libxl/libxl_internal.h | 11 +++++
>  tools/libxl/libxl_qmp.c      | 96 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 107 insertions(+)
> 
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index c453ac10a5..90ac48a659 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -417,6 +417,14 @@ _hidden int libxl__ev_qmp_send(libxl__gc *gc, 
> libxl__ev_qmp *ev,
>                                 const char *cmd, libxl__json_object *args);
>  _hidden void libxl__ev_qmp_dispose(libxl__gc *gc, libxl__ev_qmp *ev);
>  
> +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?

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

>  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
> @@ -1276,6 +1276,102 @@ int libxl__qmp_initializations(libxl__gc *gc, 
> uint32_t domid,
>      return ret;
>  }
>  
> +/* ------------ Implementation of libxl__ev_qmp ---------------- */
> +
> +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> +                               int fd, short events, short revents)
> +{
> +}
> +
> +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> +{
> +    int rc, r;
> +    struct sockaddr_un un;
> +    const char *qmp_socket_path;
> +
> +    if (ev->qmp_state != qmp_state_disconnected)
> +        return 0;
> +
> +    qmp_socket_path = libxl__qemu_qmp_path(gc, ev->domid);
> +
> +    LOGD(DEBUG, ev->domid, "Connecting to %s", qmp_socket_path);
> +
> +    libxl__carefd_begin();
> +    ev->qmp_cfd = libxl__carefd_opened(CTX, socket(AF_UNIX, SOCK_STREAM, 0));
> +    if (!ev->qmp_cfd) {
> +        LOGED(ERROR, ev->domid, "socket() failed");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +    rc = libxl_fd_set_nonblock(CTX, libxl__carefd_fd(ev->qmp_cfd), 1);
> +    if (rc)
> +        goto out;
> +
> +    rc = libxl__prepare_sockaddr_un(gc, &un, qmp_socket_path, "QMP socket");
> +    if (rc)
> +        goto out;
> +
> +    r = connect(libxl__carefd_fd(ev->qmp_cfd),
> +                (struct sockaddr *) &un, sizeof(un));
> +    if (r) {
> +        LOGED(ERROR, ev->domid, "Failed to connect to QMP socket %s",
> +              qmp_socket_path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    rc = libxl__ev_fd_register(gc, &ev->qmp_efd, qmp_ev_fd_callback,
> +                               libxl__carefd_fd(ev->qmp_cfd), POLLIN);
> +    if (rc)
> +        goto out;
> +
> +    ev->qmp_state = qmp_state_connecting;
> +    return 0;
> +
> +out:
> +    libxl__carefd_close(ev->qmp_cfd);
> +    ev->qmp_cfd = NULL;
> +    return rc;
> +}
> +
> +

Double newline.

> +/*
> + * 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);

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

> +
> +    libxl__ev_qmp_init(ev);

Thanks, Roger.

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