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

[Zhang Chen]

On 12/21/2016 10:25 PM, Eric Blake wrote:
> 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?

We can read the errp to check the last error.

> 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?

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

Thanks for your suggestion.
Zhang Chen


--
Thanks
Zhang Chen