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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, Jul 5, 2011 at 18:32, Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx> wrote:
>
>> +static void qmp_handle_error_response(libxl__qmp_handler *qmp,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âconst libxl__json_object *resp)
>> +{
>> + Â Âcallback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
> ...
>> + Â Âif (pp) {
>> + Â Â Â Âif (pp->id == qmp->wait_for_id) {
>> + Â Â Â Â Â Â/* tell that the id have been processed */
>> + Â Â Â Â Â Âqmp->wait_for_id = 0;
>> + Â Â Â Â}
>> + Â Â Â ÂSIMPLEQ_REMOVE(&qmp->callback_list, pp, callback_id_pair, next);
>> + Â Â Â Âfree(pp);
>
> I think this needs to call the callback, or the code which set up the
> callback will surely just continue to wait forever.

Ok, I will just call the callback with NULL, so, the callback will
fail explicitly. That will not change anything for the only callback I
have.

> Later: I see that you have a single wait_for_id. ÂWhat if multiple
> callers want to use a single qmp for multiple things ? ÂOr is
> "wait_for_id" simply the one that you're waiting on synchronously ?

The second.
Multiple callers can call qmp_send_synchronous multiple time.

>> +static int qmp_next(libxl__qmp_handler *qmp)
>> +{
> ...
>> + Â Â Â Âret = select(qmp->qmp_fd + 1, &rfds, NULL, NULL, &timeout);
>> + Â Â Â Âif (ret > 0) {
>> + Â Â Â Â Â Ârd = read(qmp->qmp_fd, qmp->buffer, QMP_RECEIVE_BUFFER_SIZE);
>> + Â Â Â Â Â Âif (rd > 0) {
>> + Â Â Â Â Â Â Â Âbreak;
> ...
>> + Â Âs = qmp->buffer;
>> + Â Âs_end = qmp->buffer + rd;
>> + Â Âwhile (s < s_end) {
>> + Â Â Â Âlibxl__json_object *o = libxl__json_parse(qmp->ctx, &qmp->yajl_ctx,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â&s, s_end - s);
>
> This assumes that the response will be received in a single read().
> This is not correct. Âread() may return partial results; it may also
> aggregate multiple writes from the sending qemu into a single read()
> result.

Actually, json_parse takes care of these. It tells if it's a partiel
result and how much of the buffer have been read. So the function
assume that the caller will call qmp_next again if it's needed (like
in case a callback have not been called).
I tried the function with a small buffer (not enough for an entire
response) and it works fine.

> You need to pull data into the buffer and then test the buffer for
> completeness (eg by looking for the cr-lf), and split the buffer up
> into packets yourself, and if there are partial packets left over
> go round and read again.

At least, I will check if the protocole is respected, and I will not
relie anymore on json_parse to tell me the end of a packet.

-- 
Anthony PERARD

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