[Qemu-devel] [PATCH] block: modify top-id's comments

[Qemu-devel] [PATCH] block: modify top-id's comments

[Wang WeiWei]

Kevin Wolf's advice:
http://lists.gnu.org/archive/html/qemu-devel/2016-09/msg02391.html

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
Signed-off-by: Wang WeiWei <wangww.fnst@cn.fujitsu.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qapi/block-core.json | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ada3202..a605107 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2184,7 +2184,8 @@
 # @mode: the replication mode
 #
 # @top-id: #optional In secondary mode, node name or device ID of the root
-#          node who owns the replication node chain. Ignored in primary mode.
+#          node who owns the replication node chain. Must not be given in
+#          primary mode.
 #
 # Since: 2.8
 ##
-- 
2.7.4

Re: [Qemu-devel] [Qemu-block] [PATCH] block: modify top-id's comments

[Kashyap Chamarthy]

On Fri, Sep 30, 2016 at 04:53:38PM +0800, Wang WeiWei wrote:
> Kevin Wolf's advice:
> http://lists.gnu.org/archive/html/qemu-devel/2016-09/msg02391.html

IIUC, what Stefan meant is to write a simple descriptive commit message
that explains why this change is necessary -- the above URL might have
context, but having it spelled out in the Git commit message will be
useful when someone is examining Git history offline.

I think Stefan was looking for something like (came up with this after
reading the above thread):

  QAPI: BlockdevOptionsReplication: Clarify 'top-id' parameter usage

  In primary mode, the replication driver will ignore 'top-id'
  parameter, thus it must not be supplied.  Explicitly spell that out in
  the QAPI schema for BlockdevOptionsReplication.

> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Signed-off-by: Wang WeiWei <wangww.fnst@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qapi/block-core.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ada3202..a605107 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2184,7 +2184,8 @@
>  # @mode: the replication mode
>  #
>  # @top-id: #optional In secondary mode, node name or device ID of the root
> -#          node who owns the replication node chain. Ignored in primary mode.
> +#          node who owns the replication node chain. Must not be given in
> +#          primary mode.
>  #
>  # Since: 2.8
>  ##
> -- 
> 2.7.4
> 
> 
> 
> 

-- 
/kashyap

Re: [Qemu-devel] [PATCH v2] block: modify top-id's comments

[Kevin Wolf]

This is v2, should be included in the subject line.

Am 30.09.2016 um 10:53 hat Wang WeiWei geschrieben:
> Kevin Wolf's advice:
> http://lists.gnu.org/archive/html/qemu-devel/2016-09/msg02391.html
> 
> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Signed-off-by: Wang WeiWei <wangww.fnst@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  qapi/block-core.json | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ada3202..a605107 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2184,7 +2184,8 @@
>  # @mode: the replication mode
>  #
>  # @top-id: #optional In secondary mode, node name or device ID of the root
> -#          node who owns the replication node chain. Ignored in primary mode.
> +#          node who owns the replication node chain. Must not be given in
> +#          primary mode.
>  #
>  # Since: 2.8
>  ##

As I already wrote for v1:

The matching code in block/replication.c is missing to actually error
out if the option is given in primary mode.

Kevin

Re: [Qemu-devel] [PATCH v2] block: modify top-id's comments

[wangweiwei]

Ok,thanks.
On 09/30/2016 05:52 PM, Kevin Wolf wrote:
> This is v2, should be included in the subject line.
>
> Am 30.09.2016 um 10:53 hat Wang WeiWei geschrieben:
>> Kevin Wolf's advice:
>> http://lists.gnu.org/archive/html/qemu-devel/2016-09/msg02391.html
>>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
>> Signed-off-by: Wang WeiWei <wangww.fnst@cn.fujitsu.com>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  qapi/block-core.json | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ada3202..a605107 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -2184,7 +2184,8 @@
>>  # @mode: the replication mode
>>  #
>>  # @top-id: #optional In secondary mode, node name or device ID of the root
>> -#          node who owns the replication node chain. Ignored in primary mode.
>> +#          node who owns the replication node chain. Must not be given in
>> +#          primary mode.
>>  #
>>  # Since: 2.8
>>  ##
>
> As I already wrote for v1:
>
> The matching code in block/replication.c is missing to actually error
> out if the option is given in primary mode.
>
> Kevin
>
>
> .
>

Re: [Qemu-devel] [Qemu-block] [PATCH] block: modify top-id's comments

[Stefan Hajnoczi]

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

On Fri, Sep 30, 2016 at 11:48:44AM +0200, Kashyap Chamarthy wrote:
> On Fri, Sep 30, 2016 at 04:53:38PM +0800, Wang WeiWei wrote:
> > Kevin Wolf's advice:
> > http://lists.gnu.org/archive/html/qemu-devel/2016-09/msg02391.html
> 
> IIUC, what Stefan meant is to write a simple descriptive commit message
> that explains why this change is necessary -- the above URL might have
> context, but having it spelled out in the Git commit message will be
> useful when someone is examining Git history offline.
> 
> I think Stefan was looking for something like (came up with this after
> reading the above thread):
> 
>   QAPI: BlockdevOptionsReplication: Clarify 'top-id' parameter usage
> 
>   In primary mode, the replication driver will ignore 'top-id'
>   parameter, thus it must not be supplied.  Explicitly spell that out in
>   the QAPI schema for BlockdevOptionsReplication.

Yes.  Posting just a URL as the commit description is not enough.

I tried to explain the purpose of commit messages/descriptions in my
previous reply.

You can read a lot more about it elsewhere too:

http://chris.beams.io/posts/git-commit/
(The key points are the introduction and "7. Use the body to explain
what and why vs. how")

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH] block: modify top-id's comments

[wangweiwei]

Ok, I'wll wrap it.
On 09/29/2016 09:35 PM, Eric Blake wrote:
> On 09/28/2016 05:31 AM, Stefan Hajnoczi wrote:
>> On Wed, Sep 28, 2016 at 01:30:39PM +0800, Wang WeiWei wrote:
>>
>> Please send a proper commit message and commit description.  "modify
>> top-id's comments" says almost nothing and there is no commit
>> description.  I don't know why you are making this change so I can't
>> review and apply it.
>>
>
>>> +++ b/qapi/block-core.json
>>> @@ -2184,7 +2184,7 @@
>>>  # @mode: the replication mode
>>>  #
>>>  # @top-id: #optional In secondary mode, node name or device ID of the root
>>> -#          node who owns the replication node chain. Ignored in primary mode.
>>> +#          node who owns the replication node chain. Must not be given in primary mode.
>
> That, and your line exceeds 80 columns; please wrap it.
>

Re: [Qemu-devel] [Qemu-block] [PATCH] block: modify top-id's comments

[Eric Blake]

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

On 09/28/2016 05:31 AM, Stefan Hajnoczi wrote:
> On Wed, Sep 28, 2016 at 01:30:39PM +0800, Wang WeiWei wrote:
> 
> Please send a proper commit message and commit description.  "modify
> top-id's comments" says almost nothing and there is no commit
> description.  I don't know why you are making this change so I can't
> review and apply it.
> 

>> +++ b/qapi/block-core.json
>> @@ -2184,7 +2184,7 @@
>>  # @mode: the replication mode
>>  #
>>  # @top-id: #optional In secondary mode, node name or device ID of the root
>> -#          node who owns the replication node chain. Ignored in primary mode.
>> +#          node who owns the replication node chain. Must not be given in primary mode.

That, and your line exceeds 80 columns; please wrap it.

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

Re: [Qemu-devel] [Qemu-block] [PATCH] block: modify top-id's comments

[Stefan Hajnoczi]

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

On Wed, Sep 28, 2016 at 01:30:39PM +0800, Wang WeiWei wrote:

Please send a proper commit message and commit description.  "modify
top-id's comments" says almost nothing and there is no commit
description.  I don't know why you are making this change so I can't
review and apply it.

> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com>
> Signed-off-by: Wang WeiWei <wangww.fnst@cn.fujitsu.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qapi/block-core.json | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ada3202..0935b81 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2184,7 +2184,7 @@
>  # @mode: the replication mode
>  #
>  # @top-id: #optional In secondary mode, node name or device ID of the root
> -#          node who owns the replication node chain. Ignored in primary mode.
> +#          node who owns the replication node chain. Must not be given in primary mode.
>  #
>  # Since: 2.8
>  ##
> -- 
> 1.9.3
> 
> 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]