Re: [Qemu-devel] [PATCH for-2.9 V4 2/2] Add a new qmp command to do checkpoint, get replication error

[addr_cc]

On 12/22/2016 12:08 AM, Zhang Chen wrote:
>>> Make sense, this command trying to collect status on whether
>>> an error has occurred, and the "replication_get_error_all(errp)"
>>> is always succeeds. So, Can you suggest to me the right name?
>> If replication_get_error_all() always succeeds, then what failure is
>> possible to be checking for?
> 
> We can read the errp to check the last error.

But turning around and reporting an error to the caller is not nice.
The caller can't distinguish between "I called the command correctly,
and it is telling me the system has encountered a replication error" and
"I called the command incorrectly, and it is telling me my usage is
wrong even though the system has never encountered a replication error".
 Passing information through errp is NOT the right way to successfully
report status.

> 
>>
>> Maybe the problem is deeper, in that replication_get_error_all() has an
>> unusual signature, and needs to be fixed first.  I don't know, and
>> haven't looked; I'm only coming at this from the user interface
>> perspective.  But it makes no sense to have a command that queries
>> whether an error occurred, but where an error having occurred is fatal
>> (you want the command to successfully report that an error has occurred,
>> not error out with a second error because a first error was present).
> 
> Do you means we should fix "void replication_get_error_all()"
> to "int replication_get_error_all()" first for get the return value?

Quite possibly yes. But maybe you don't have to do that, and can come up
with a scheme where only the QMP command wrapper has to be careful.
Perhaps something like this would work:

>> Then you probably want a query style interface:
>>
>> { 'command': 'query-xen-replication-status',
>>    'returns': 'SomeStruct' }
>>
>> where SomeStruct contains details such as status (perhaps an enum that
>> reports 'normal' or 'error'), and where you are free to add additional
>> pieces of information that may prove useful later (rather than having to
>> invent yet more commands that give only a boolean result of success or
>> failure based on whether the state is normal or in error).

SomeStruct *qmp_query_xen_replication_status(Error **errp)
{
    Error *err = NULL;
    SomeStruct *result = g_new0(SomeStruct, 1);
    replication_get_error_all(&err);
    result.state = err ? SOME_ENUM_ERRORED : SOME_ENUM_NORMAL;
    error_free(err);
    /* ... and now you can add additional status items to the API,
       as needed. errp remains unset, because the command succeeds */
}

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature