qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
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

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