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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Jun 28, 2011 at 10:14, Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx> wrote:
> 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).

I will look at this patch.

>> >> +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?

Sorry, I should say replace by macros, instead of "remove". Is it fine
to keep the #ifdef inside the struct or should I remove the #ifdef and
keep "yajl_gen g" field even if it will not be used (in case
DEBUG_ANSWER in undef) ?

> 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?

This should not be necessary as the read size (rd) is always used.

> 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);
> ?

This printf seams to work better with "%.*s". I will use that, so no
more zero-terminited buffer and no more buffer overrun.

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

Thanks,

-- 
Anthony PERARD

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