From: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
To: Eric Blake <eblake@redhat.com>,
qemu devel <qemu-devel@nongnu.org>,
Jason Wang <jasowang@redhat.com>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>,
zhanghailiang <zhang.zhanghailiang@huawei.com>,
"eddie . dong" <eddie.dong@intel.com>,
Bian Naimeng <biannm@cn.fujitsu.com>,
Changlong Xie <xiecl.fnst@cn.fujitsu.com>,
Wen Congyang <wencongyang@gmail.com>
Subject: Re: [Qemu-devel] [PATCH for-2.9 V4 2/2] Add a new qmp command to do checkpoint, get replication error
Date: Thu, 22 Dec 2016 14:08:33 +0800 [thread overview]
Message-ID: <ed8e4268-e515-470c-9e8f-a0194e43100b@cn.fujitsu.com> (raw)
In-Reply-To: <acec3060-2325-d92a-8f67-20a8ae1222cc@redhat.com>
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
next prev parent reply other threads:[~2016-12-22 6:08 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-16 2:46 [Qemu-devel] [PATCH for-2.9 V4 0/2] Add new qmp commands to suppurt Xen COLO Zhang Chen
2016-12-16 2:46 ` [Qemu-devel] [PATCH for-2.9 V4 1/2] Add a new qmp command to start/stop replication Zhang Chen
2016-12-20 14:38 ` Eric Blake
2016-12-21 0:14 ` Stefano Stabellini
2016-12-16 2:46 ` [Qemu-devel] [PATCH for-2.9 V4 2/2] Add a new qmp command to do checkpoint, get replication error Zhang Chen
2016-12-20 14:42 ` Eric Blake
2016-12-21 6:56 ` Zhang Chen
2016-12-21 14:25 ` Eric Blake
2016-12-22 6:08 ` Zhang Chen [this message]
2016-12-22 12:48 ` addr_cc
2016-12-22 12:52 ` Eric Blake
2016-12-23 6:05 ` Zhang Chen
2016-12-21 0:18 ` Stefano Stabellini
2016-12-21 7:10 ` Zhang Chen
2016-12-20 0:56 ` [Qemu-devel] [PATCH for-2.9 V4 0/2] Add new qmp commands to suppurt Xen COLO Stefano Stabellini
2016-12-20 2:34 ` Zhang Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ed8e4268-e515-470c-9e8f-a0194e43100b@cn.fujitsu.com \
--to=zhangchen.fnst@cn.fujitsu.com \
--cc=biannm@cn.fujitsu.com \
--cc=eblake@redhat.com \
--cc=eddie.dong@intel.com \
--cc=jasowang@redhat.com \
--cc=lizhijian@cn.fujitsu.com \
--cc=qemu-devel@nongnu.org \
--cc=wencongyang@gmail.com \
--cc=xiecl.fnst@cn.fujitsu.com \
--cc=zhang.zhanghailiang@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).