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

To: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH V5 3/3] libxl, Introduce a QMP client
From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
Date: Tue, 28 Jun 2011 15:22:41 +0100
Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>, Xen Devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Tue, 28 Jun 2011 07:23:56 -0700
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:from:date :x-google-sender-auth:message-id:subject:to:cc:content-type :content-transfer-encoding; bh=34+pi+4bxQeVk9qjciGwt1TsmSkvbmiKbcB7DgDUpyc=; b=G+/5hZ7mCd62snBcmF6Pdt3I4aJJ1+urckDNQMrk6k3H91Nbx09/Xp+EaTg3jFNM6g l4Gav9eUfkSQ6tQxt9XlrgfdKFxAoBS7RVuIshpVNkAQn1Xz7tR3DiBQFppw7RlAOn/U 6ZL5Lg3o+cL41UdrRr467m5It+ZOjohwWtSck=
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1309252470.32717.285.camel@xxxxxxxxxxxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1309187551-21538-1-git-send-email-anthony.perard@xxxxxxxxxx> <1309187551-21538-4-git-send-email-anthony.perard@xxxxxxxxxx> <19976.44330.128977.805588@xxxxxxxxxxxxxxxxxxxxxxxx> <BANLkTin=_y1jwx4XFcOgjBcEs2y4r+40Ow@xxxxxxxxxxxxxx> <1309252470.32717.285.camel@xxxxxxxxxxxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
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