Re: [Xen-devel] [PATCH v5 11/15] libxl_dm: Pre-open QMP socket for QEMU

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v5 11/15] libxl_dm: Pre-open QMP socket for 
QEMU"):
> 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.

Thanks for this.  I have only tiny comments about this.  (I checked the
error handling patterns and they seemed conventional and correct.)

FAOD, and I think this is not entirely clear from your commit message:

AIUI the overall effect of this patch is not to enable any new
functionality and not to fix any bug.  It just moves the qemu socket
creation from qemu to libxl, but nothing in libxl relies on this yet.

Am I right ?  If so please put that in the commit message.

> +static int libxl__pre_open_qmp_socket(libxl__gc *gc, int domid, int *fd_r)
...
> +    if (bind(fd, (struct sockaddr*) &un, sizeof(un)) < 0) {
> +        LOGED(ERROR, domid, "bind('%s') failed", path);
> +        rc = ERROR_FAIL;
> +        goto out;
> +    }

From libxl/CODING_STYLE:

    * Function calls which might fail (ie most function calls) are
      handled by putting the return/status value into a variable, and
      then checking it in a separate statement:
              char *dompath = libxl__xs_get_dompath(gc, bl->domid);
              if (!dompath) { rc = ERROR_FAIL; goto out; }

This needs changing throughout the series, I'm afraid.

>  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)
...
> -                     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

                                              needs

> +     * the new interface where we can pre-open the QMP socket. */
> +    if (libxl_defbool_val(b_info->dm_restrict))
> +    {

Misplaced brace, should be end of the previous line.

> @@ -1739,10 +1796,11 @@ static int libxl__build_device_model_args(libxl__gc 
> *gc,
>      case LIBXL_DEVICE_MODEL_VERSION_QEMU_XEN:
>          assert(dm_state_fd != NULL);
>          assert(*dm_state_fd < 0);
> +        assert(dm_monitor_fd != NULL);

Jolly good.  But I would be tempted to move or copy this assert to
libxl__pre_open_qmp_socket.  What do you think ?

>      ret = libxl__build_device_model_args(gc, "stubdom-dm", guest_domid,
>                                           guest_config, &args, NULL,
> -                                         d_state, NULL);
> +                                         d_state, NULL, NULL);

Did you consider adding dm_monitor_fd to d_state ?

Ian.

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