Re: [Xen-devel] [PATCH v5 02/15] libxl_qmp: Connect to QMP socket

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v5 02/15] libxl_qmp: Connect to QMP socket"):
> This is a first patch to implement libxl__ev_qmp, it only connects to
> the QMP socket of QEMU and registers a fd callback that does nothing.
...
> +typedef enum {
> +    /* initial state */
> +    qmp_state_disconnected = 1,
> +    /* connected to QMP socket, waiting for greeting message */
> +    qmp_state_connecting,
> +    /* greeting message received */
> +    qmp_state_greeting,
> +    /* qmp_capabilities command sent, waiting for reply */
> +    qmp_state_capability_negotiation,
> +    /* ready to send commands */
> +    qmp_state_connected,
> +} libxl__qmp_state;

IWBN to relate these private states to the outward-facing API states
like `Connected'.

I often write a table of legal field values - see for example,
libxl_exec.c near l.241 and libxl_stream_read.c near l.35.  But maybe
this is not complicated enough to need that.

> +static int qmp_ev_connect(libxl__gc *gc, libxl__ev_qmp *ev)
> +{
...
> +    r = connect(libxl__carefd_fd(ev->qmp_cfd),
> +                (struct sockaddr *) &un, sizeof(un));

Up to here:

Acked-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>

But:

> +int libxl__ev_qmp_send(libxl__gc *gc, libxl__ev_qmp *ev,
> +                       const char *cmd, libxl__json_object *args)
> +{
> +    LOGD(DEBUG, ev->domid, " ev %p, cmd '%s'", ev, cmd);
> +
> +    /* Connect to QEMU if not already connected */
> +    return qmp_ev_connect(gc, ev);
> +}

I think it would be nicer to read a complete implementation of this
function.  Right now it's obviously wrong and impossible to review.

So please postpone this hunk.

Thanks,
Ian.

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