Re: [Xen-devel] [PATCH v6 09/11] libxl_qmp: Store advertised QEMU version in libxl__ev_qmp

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Anthony PERARD writes ("[PATCH v6 09/11] libxl_qmp: Store advertised QEMU 
version in libxl__ev_qmp"):
> This will be used in a later patch.

Thanks.  I have one comment about defensive programming in the macro,
and a couple of formatting nits.

> +        /*
> +         * Store advertised QEMU version
> +         * { "QMP": { "version": {
> +         *     "qemu": { "major": int, "minor": int, "micro": int } } } }
> +         */
> +        o = libxl__json_map_get("QMP", resp, JSON_MAP);
> +        o = libxl__json_map_get("version", o, JSON_MAP);
> +        o = libxl__json_map_get("qemu", o, JSON_MAP);
> +#define GRAB_VERSION(level) \
> +    ev->qemu_version.level = libxl__json_object_get_integer( \
> +        libxl__json_map_get(#level, o, JSON_INTEGER));
> +        GRAB_VERSION(major);

Your GRAB_VERSION macro ought to have a pair of ( ) around it, or the
do{...}while(0) trick.  I would prefer the indentation to be such that
the statement inside the macro is indented like the ones outside.

> +        GRAB_VERSION(minor);
> +        GRAB_VERSION(micro);
> +#undef GRAB_VERSION
> +        LOGD(DEBUG, ev->domid, "QEMU version: %d.%d.%d",
> +             ev->qemu_version.major, ev->qemu_version.minor,
> +             ev->qemu_version.micro);

A very minor point, but if you broke the line after `major,' this
would make the three identical things more similar-looking.

Thanks,
Ian.

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