Re: [Xen-devel] [PATCH V5 3/3] libxl, Introduce a QMP client

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Mon, 2011-06-27 at 18:04 +0100, Anthony PERARD wrote:
> On Mon, Jun 27, 2011 at 17:17, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
> > Anthony PERARD writes ("[Xen-devel] [PATCH V5 3/3] libxl, Introduce a QMP 
> > client"):
> >> QMP stands for QEMU Monitor Protocol and it is used to query information
> >> from QEMU or to control QEMU.
> >>
> >> This implementation will ask QEMU the list of chardevice and store the
> >> path to serial0 in xenstored. So we will be able to use xl console with
> >> QEMU upstream.
> >
> > Can I make a suggestion ?  I think the formulaic json parser stuff
> > could usefully live in a separate file.
> 
> Ok, I will cut the file.

FWIW my "libxl: IDL: autogenerate functions to produce JSON from libxl
data structures" patch added libxl_json.c. If you want to add that file
with the bits you need I will rebase onto it (since I need to rework at
least this last patch of my series anyway, see below).

> >> +static inline yajl_gen_status yajl_gen_asciiz(yajl_gen hand, const
> > char *str)
> >
> > Isn't this a hostage to fortune ?  yajl may grow an identically-named
> > function in which case this will no longer build.
> 
> Maybe. I will rename to libxl__yajl_gen_asciiz.

Good idea. My patch also added yajl_gen_asciiz. I shall defer to the
version which you will have added when I rebase.

> >> +#ifdef DEBUG_ANSWER
> >> +    if (qmp->g)
> >> +        yajl_gen_free(qmp->g);
> >> +#endif
> >
> > Can this #ifdef not be shuffled off somewhere ?  Ie, make a debug
> > function (or macro) which we call unconditionally.
> 
> Ok, I will remove all the #ifdef debug_answer.

I think Ian was only suggesting to remove the ifdef's from the main flow
of code and you've done that for most of the uses with your DEBUG_GEN*
but here perhaps you could also define and call functions which are
empty in the non-debug case. e.g. DEBUG_GEN_START, DEBUG_GEN_END,
DEBUG_GEN_REPORT?

BTW I noticed:
+#ifdef DEBUG_RECEIVED
+    qmp->buffer[rd] = 0;
+    LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG, "received: '%s'", qmp->buffer);
+#endif

I wondered if the zero termination of the string should be there even in
the non-debug case? Also is there a buffer overrun (when
rd==QMP_RECEIVE_BUFFER_SIZE)?

If its the latter you could perhaps use
        printf("%*s", rd, qmp->buf);
?

Having done that then:
        #define DEBUG_RECEIVED(qmp) LIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG...)
Would remove this ifdef from the codeflow too...

> > The rest looks plausible.
> 
> Thanks for the review,
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel