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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

On 12/21/2016 12:56 AM, Zhang Chen wrote:

>>>   +void qmp_xen_get_replication_error(Error **errp)
>>> +    {
>>> +        replication_get_error_all(errp);
>>> +    }
>> Is this trying to cause a replication error, or is it trying to collect
>> status on whether an error has occurred? The name 'get' usually implies
>> a query that does not change state, but a query usually needs some way
>> to return what was queried back to the user, so a void function with no
>> parameters other than error is odd (either the command succeeds because
>> there was no error, or the command fails with an error message because
>> the query had something to get?).
> 
> 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?

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).


>>> +# Since: 2.9
>>> +##
>>> +{ 'command': 'xen-get-replication-error' }
>> So I don't think this is the right name for its signature, but I don't
>> know what you are trying to accomplish with this command to suggest the
>> right name.
> 
> This interface is called to check if error happened in replication.

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).


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]