Re: [Xen-devel] [PATCH v4 27/32] libxl: QEMU startup sync based on QMP

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

On Fri, Jul 27, 2018 at 03:06:09PM +0100, Anthony PERARD wrote:
> This is only activated when dm_restrict=1, as explained in the previous
> patch "libxl_dm: Pre-open QMP socket for QEMU"
> 
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Just one nit...

> ---
> 
> Notes:
>     v4:
>         moved to libxl__dm_spawn_* from libxl__spawn_*
> 
>  tools/libxl/libxl_dm.c       | 52 ++++++++++++++++++++++++++++++++++++
>  tools/libxl/libxl_internal.h |  1 +
>  2 files changed, 53 insertions(+)
> 
> diff --git a/tools/libxl/libxl_dm.c b/tools/libxl/libxl_dm.c
> index 9e3e501457..0c11e96a5d 100644
> --- a/tools/libxl/libxl_dm.c
> +++ b/tools/libxl/libxl_dm.c
> @@ -2290,6 +2290,8 @@ static void device_model_startup_failed(libxl__egc *egc,
>                                          int rc);
>  static void device_model_detached(libxl__egc *egc,
>                                    libxl__spawn_state *spawn);
> +static void device_model_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
> +                                const libxl__json_object *response, int rc);
>  
>  /* our "next step" function, called from those callbacks and elsewhere */
>  static void device_model_spawn_outcome(libxl__egc *egc,
> @@ -2421,6 +2423,17 @@ retry_transaction:
>      spawn->failure_cb = device_model_startup_failed;
>      spawn->detached_cb = device_model_detached;
>  
> +    libxl__ev_qmp_init(&dmss->qmp);
> +    if (dm_monitor_fd >= 0) {
> +        /* There is a valid QMP socket available now,
> +         * use it to find out when QEMU is ready */
> +        dmss->qmp.callback = device_model_qmp_cb;
> +        dmss->qmp.domid = domid;
> +        dmss->qmp.cfd = NULL;
> +        rc = libxl__ev_qmp_send(gc, &dmss->qmp, "query-status", NULL);
> +        if (rc) goto out_close;
> +    }
> +
>      rc = libxl__spawn_spawn(egc, spawn);
>      if (rc < 0)
>          goto out_close;
> @@ -2490,6 +2503,43 @@ static void device_model_detached(libxl__egc *egc,
>      device_model_spawn_outcome(egc, dmss, 0);
>  }
>  
> +static void device_model_qmp_cb(libxl__egc *egc, libxl__ev_qmp *ev,
> +                                const libxl__json_object *response,
> +                                int rc)
> +{
> +    EGC_GC;
> +    libxl__dm_spawn_state *dmss = CONTAINER_OF(ev, *dmss, qmp);
> +    const libxl__json_object *o;
> +    const char *status;
> +
> +    libxl__ev_qmp_dispose(gc, ev);
> +
> +    if (rc)
> +        goto failed;
> +
> +    o = libxl__json_map_get("status", response, JSON_STRING);
> +    if (!o) {
> +        LOGD(DEBUG, ev->domid, "QMP unexpected response");
> +        rc = ERROR_FAIL;
> +        goto failed;
> +    }
> +    status = libxl__json_object_get_string(o);
> +    if (!strcmp(status, "running")) {
> +        /* success */
> +    } else {
> +        LOGD(DEBUG, ev->domid, "Unexpected QEMU status: %s", status);
> +        rc = ERROR_FAIL;
> +        goto failed;
> +    }

Isn't it clearer to just use:

if (strcmp(status, "running")) {
    /* Failure. */
}

The empty success branch doesn't look very useful.

Thanks, Roger.

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