Re: [Xen-devel] [PATCH 5/7] libxl_qmp, Return the callback return code in qmp_next.

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Oct 7, 2011 at 13:36, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Fri, 2011-10-07 at 13:10 +0100, Anthony PERARD wrote:
>> So, if there is an error in the answer given by QEMU, the function
>> qmp_synchronous_send while know.
> Â Â Â Â Â Â Â Â Â Â Â "will"
>
>>
>> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
>> ---
>> Âtools/libxl/libxl_qmp.c | Â 14 ++++++++------
>> Â1 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_qmp.c b/tools/libxl/libxl_qmp.c
>> index 1594a4f..cd3e4e4 100644
>> --- a/tools/libxl/libxl_qmp.c
>> +++ b/tools/libxl/libxl_qmp.c
>> @@ -233,6 +233,7 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â const libxl__json_object *resp)
>> Â{
>> Â Â Âlibxl__qmp_message_type type = LIBXL__QMP_MESSAGE_TYPE_INVALID;
>> + Â Âint rc = 0;
>>
>> Â Â Âtype = qmp_response_type(qmp, resp);
>> Â Â ÂLIBXL__LOG(qmp->ctx, LIBXL__LOG_DEBUG,
>> @@ -241,14 +242,14 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
>> Â Â Âswitch (type) {
>> Â Â Âcase LIBXL__QMP_MESSAGE_TYPE_QMP:
>> Â Â Â Â Â/* On the greeting message from the server, enable QMP capabilities 
>> */
>> - Â Â Â Âenable_qmp_capabilities(qmp);
>> + Â Â Â Ârc = enable_qmp_capabilities(qmp);
>> Â Â Â Â Âbreak;
>> Â Â Âcase LIBXL__QMP_MESSAGE_TYPE_RETURN: {
>> Â Â Â Â Âcallback_id_pair *pp = qmp_get_callback_from_id(qmp, resp);
>>
>> Â Â Â Â Âif (pp) {
>> Â Â Â Â Â Â Âif (pp->callback) {
>> - Â Â Â Â Â Â Â Âpp->callback(qmp,
>> + Â Â Â Â Â Â Â Ârc = pp->callback(qmp,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â libxl__json_map_get("return", resp, JSON_ANY),
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â pp->opaque);
>> Â Â Â Â Â Â Â}
>> @@ -263,13 +264,13 @@ static int qmp_handle_response(libxl__qmp_handler *qmp,
>> Â Â Â}
>> Â Â Âcase LIBXL__QMP_MESSAGE_TYPE_ERROR:
>> Â Â Â Â Âqmp_handle_error_response(qmp, resp);
>> - Â Â Â Âbreak;
>> + Â Â Â Âreturn -1;
>
> A mixture or "return -1" and a rc variable returned at the outermost
> level is a bit odd. You should either always set rc and fall through or
> always return at the end of each case.

OK, I will change that.

>> Â Â Âcase LIBXL__QMP_MESSAGE_TYPE_EVENT:
>> Â Â Â Â Âbreak;
>> Â Â Âcase LIBXL__QMP_MESSAGE_TYPE_INVALID:
>> Â Â Â Â Âreturn -1;
>> Â Â Â}
>> - Â Âreturn 0;
>> + Â Âreturn rc;
>> Â}
>>
>> Â/*
>> @@ -358,6 +359,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler 
>> *qmp)
>>
>> Â Â Âchar *incomplete = NULL;
>> Â Â Âsize_t incomplete_size = 0;
>> + Â Âint rc = 0;
>>
>> Â Â Âdo {
>> Â Â Â Â Âfd_set rfds;
>> @@ -416,7 +418,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler 
>> *qmp)
>> Â Â Â Â Â Â Â Â Âs = end + 2;
>>
>> Â Â Â Â Â Â Â Â Âif (o) {
>> - Â Â Â Â Â Â Â Â Â Âqmp_handle_response(qmp, o);
>> + Â Â Â Â Â Â Â Â Â Ârc = qmp_handle_response(qmp, o);
>
> If rc now indicates error do we need to bail straight away or need to
> keep going around this loop? (Or is it certain we will immediately fall
> out of the loop after this?)

We can not be sure that we will return, because it could be another
message in the butffer. So I should return if there is a protocol
error. But I think that I should keep seperate the return code of a
callback, so only the interested function (qmp_synchronous_send) will
read it (and return to the caller).

>> Â Â Â Â Â Â Â Â Â Â Âlibxl__json_object_free(gc, o);
>> Â Â Â Â Â Â Â Â Â} else {
>> Â Â Â Â Â Â Â Â Â Â ÂLIBXL__LOG(qmp->ctx, LIBXL__LOG_ERROR,
>> @@ -429,7 +431,7 @@ static int qmp_next(libxl__gc *gc, libxl__qmp_handler 
>> *qmp)
>> Â Â Â Â Â} while (s < s_end);
>> Â Â } while (s < s_end);
>>
>> - Â Âreturn 1;
>> + Â Âreturn rc;
>> Â}
>>
>> Âstatic int qmp_send(libxl__qmp_handler *qmp,

-- 
Anthony PERARD

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