WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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