Re: [Xen-devel] [PATCH v5 12/15] libxl: QEMU startup sync based on QMP

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v5 12/15] libxl: QEMU startup sync based on 
QMP"):
> This is only activated when dm_restrict=1, as explained in the previous
> patch "libxl_dm: Pre-open QMP socket for QEMU"
...
> +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);

I missed who owns the memory for `response' ?  In the absence of any
indication to the contrary I think it should come from the EGC_GC.

...

Just checked qmp_ev_callback_readable and this is indeed true.  Jolly
good.

This actual functional change is pleasingly straightforward.

> +    if (rc)
> +        goto failed;
> +
> +    o = libxl__json_map_get("status", response, JSON_STRING);
> +    if (!o) {
> +        LOGD(DEBUG, ev->domid, "QMP unexpected response");

I think many of these DEBUG should be ERROR and many of these
ERROR_FAIL should be ERROR_QEMU_DID_SOME_WRONG_THING.

It's rather odd that neither this patch, nor anything that follows,
does not stop libxl from also watching the xenstore path.  I think it
would be better to suppress that rather than leave vestigial
behaviour.  Can you add patch(es) to do that, right after this one ?

You may need to add a patch to tolerate xspath==0 in the spawn code.

Thanks,
Ian.

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