Re: [Xen-devel] [PATCH v4 26/32] libxl_dm: Pre-open QMP socket for QEMU

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

On Fri, Jul 27, 2018 at 03:06:08PM +0100, Anthony PERARD wrote:
> When starting QEMU with dm_restrict=1, pre-open the QMP socket before
> exec QEMU. That socket will be usefull to findout if QEMU is ready, and
> pre-opening it means that libxl can connect to it without waiting for
> QEMU to create it.
> 
> The pre-openning is conditionnal, based on the use of dm_restrict
> because it is using a new command line option of QEMU, and dm_restrict
> support in QEMU is newer.
> 
> -chardev socket,fd=X is available with QEMU 2.12, since commit:
> > char: allow passing pre-opened socket file descriptor at startup
> > 0935700f8544033ebbd41e1f13cd528f8a58d24d
> 
> dm_restrict will be available in QEMU 3.0.
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> ---
> 
> Notes:
>     v4:
>         separate the logic to open a socket into a function.
>         Use libxl__prepare_sockaddr_un() to check path size
> 
>  tools/libxl/libxl_dm.c | 86 +++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 77 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 5c28a0ced4..9e3e501457 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -24,6 +24,8 @@
>  #include <sys/types.h>
>  #include <pwd.h>
>  #include <grp.h>
> +#include <sys/socket.h>

sys/socket.h should be already included by libxl_internal.h

> +#include <sys/un.h>

I would probably consider adding sys/un.h to libxl_internal.h unless
it's only required for this specific file.

>  
>  static const char *libxl_tapif_script(libxl__gc *gc)
>  {
> @@ -915,12 +917,58 @@ const char *libxl__qemu_qmp_path(libxl__gc *gc, int 
> domid)
>      return GCSPRINTF("%s/qmp-libxl-%d", libxl__run_dir_path(), domid);
>  }
>  
> +static int libxl__pre_open_qmp_socket(libxl__gc *gc, int domid, int *fd_r)
> +{
> +    int rc;
> +    int fd = -1;

There's no need to init fd to -1.

> +    struct sockaddr_un un;
> +    const char *path;
> +
> +    path = libxl__qemu_qmp_path(gc, domid);

You can init it a definition time.

> +
> +    fd = socket(AF_UNIX, SOCK_STREAM, 0);
> +    if (fd < 0) {
> +        LOGED(ERROR, domid, "socket() failed");
> +        return ERROR_FAIL;
> +    }
> +
> +    rc = libxl__prepare_sockaddr_un(gc, &un, path, "QEMU's QMP socket");
> +    if (rc)
> +        goto out;
> +
> +    if (unlink(path) < 0 && errno != ENOENT) {
> +        LOGED(ERROR, domid, "unlink('%s') failed", path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }

You can use libxl__remove_file which also handles EINTR.

> +
> +    if (bind(fd, (struct sockaddr*) &un, sizeof(un)) < 0) {
> +        LOGED(ERROR, domid, "bind('%s') failed", path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    if (listen(fd, 1) < 0) {
> +        LOGED(ERROR, domid, "listen() failed");
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }
> +
> +    *fd_r = fd;
> +    rc = 0;
> +
> +out:
> +    if (rc && fd >= 0)
> +        close(fd);
> +    return rc;
> +}
> +
>  static int libxl__build_device_model_args_new(libxl__gc *gc,
>                                          const char *dm, int guest_domid,
>                                          const libxl_domain_config 
> *guest_config,
>                                          char ***args, char ***envs,
>                                          const libxl__domain_build_state 
> *state,
> -                                        int *dm_state_fd)
> +                                        int *dm_state_fd, int *dm_monitor_fd)
>  {
>      const libxl_domain_create_info *c_info = &guest_config->c_info;
>      const libxl_domain_build_info *b_info = &guest_config->b_info;
> @@ -949,10 +997,25 @@ static int libxl__build_device_model_args_new(libxl__gc 
> *gc,
>                        GCSPRINTF("%d", guest_domid), NULL);
>  
>      flexarray_append(dm_args, "-chardev");
> -    flexarray_append(dm_args,
> -                     GCSPRINTF("socket,id=libxl-cmd,"
> -                               "path=%s,server,nowait",
> -                               libxl__qemu_qmp_path(gc, guest_domid)));
> +    /* If we have to use dm_restrict, QEMU need to be new enough and will 
> have
> +     * the new interface where we can pre-open the QMP socket. */
> +    if (libxl_defbool_val(b_info->dm_restrict))
> +    {
> +        int rc;
> +
> +        rc = libxl__pre_open_qmp_socket(gc, guest_domid, dm_monitor_fd);
> +        if (rc)
> +            return rc;
> +
> +        flexarray_append(dm_args,
> +                         GCSPRINTF("socket,id=libxl-cmd,fd=%d,server,nowait",
> +                                   *dm_monitor_fd));
> +    } else {
> +        flexarray_append(dm_args,
> +                         GCSPRINTF("socket,id=libxl-cmd,"
> +                                   "path=%s,server,nowait",
> +                                   libxl__qemu_qmp_path(gc, guest_domid)));
> +    }
>  
>      flexarray_append(dm_args, "-no-shutdown");
>      flexarray_append(dm_args, "-mon");
> @@ -1731,7 +1794,8 @@ static int libxl__build_device_model_args(libxl__gc *gc,
>                                          const libxl_domain_config 
> *guest_config,
>                                          char ***args, char ***envs,
>                                          const libxl__domain_build_state 
> *state,
> -                                        int *dm_state_fd)
> +                                        int *dm_state_fd,
> +                                        int *dm_monitor_fd)

You cna place dm_monitor_fd on the same line.

Thanks, Roger.

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