Re: [Xen-devel] [PATCH v6.1 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v6.1 05/11] libxl_qmp: Implementation of 
libxl__ev_qmp_*"):
...

I think this was intended to satisfy my request for comments about
legal states:

> +/* helpers */
> +
> +static void qmp_ev_ensure_reading_writing(libxl__gc *gc, libxl__ev_qmp *ev)
> +{
> +    bool enable = false;

This one is probably an asisstant for transitioning between states so
the pre- and post-conditions may not be pure.  Whatever it is should
be documented...

> +static void qmp_ev_set_state(libxl__gc *gc, libxl__ev_qmp *ev,
> +                             libxl__qmp_state new_state)
> +{

This one at least does not need a comment :-).

> +static int qmp_ev_prepare_cmd(libxl__gc *gc,
> +                              libxl__ev_qmp *ev,
> +                              const char *cmd,
> +                              const libxl__json_object *args)
> +{
> +    char *buf = NULL;

Missing state comment.

> +static void qmp_ev_fd_callback(libxl__egc *egc, libxl__ev_fd *ev_fd,
> +                               int fd, short events, short revents)
> +{

Missing state comment, although I think the precondition can be easily
inferred from the state of ev_fd and the postcondition varies, but
would still be nice to discuss it.

> +static int qmp_ev_callback_writable(libxl__gc *gc,
> +                                    libxl__ev_qmp *ev, int fd)
> +    /* connected -> waiting_reply
> +     * the state isn't change otherwise. */
> +{

I don't know what `otherwise' means.  Maybe you mean all other states
are legal and remain unchanged ?  But that does not seem to be
likely.  Presumably disconnected is ruled out, at least.

> +static int qmp_ev_callback_readable(libxl__egc *egc,
> +                                    libxl__ev_qmp *ev, int fd)
> +    /* on error: * -> disconnected */

Precondition ?  And on non-error ?

> +static int qmp_ev_get_next_msg(libxl__egc *egc, libxl__ev_qmp *ev,
> +                               libxl__json_object **o_r)
> +    /* Find a JSON object and store it in o_r.
> +     * return ERROR_NOTFOUND if no object is found.
> +     * `o_r` is allocated within `egc`.
> +     */
> +{

Missing state comment.

> +static int qmp_ev_parse_error_messages(libxl__egc *egc,
> +                                       libxl__ev_qmp *ev,
> +                                       const libxl__json_object *resp)
> +{

This doesn't touch the state I think.  Should perhaps be mentioned in
a comment.

Thanks,
Ian.

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