[RFC qemu 0/6] mirror: implement incremental and bitmap modes

Fabian Grünbichler posted 6 patches 4 years, 3 months ago
Test docker-quick@centos7 passed
Test FreeBSD passed
Test docker-mingw@fedora passed
Test checkpatch failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20200218100740.2228521-1-f.gruenbichler@proxmox.com
Maintainers: Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>, John Snow <jsnow@redhat.com>
There is a newer version of this series
include/block/block_int.h   |    4 +-
block/mirror.c              |   96 +-
blockdev.c                  |   71 +-
tests/test-block-iothread.c |    4 +-
qapi/block-core.json        |   29 +-
tests/qemu-iotests/284      |  547 +++++++
tests/qemu-iotests/284.out  | 2846 +++++++++++++++++++++++++++++++++++
tests/qemu-iotests/group    |    1 +
8 files changed, 3567 insertions(+), 31 deletions(-)
create mode 100755 tests/qemu-iotests/284
create mode 100644 tests/qemu-iotests/284.out
[RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Fabian Grünbichler 4 years, 3 months ago
picking up on John's in-progress patch series from last summer, this is
a stab at rebasing and adding test cases for the low-hanging fruits:

- bitmap mirror mode with always/on-success/never bitmap sync mode
- incremental mirror mode as sugar for bitmap + on-success

Fabian Grünbichler (4):
  mirror: add check for bitmap-mode without bitmap
  mirror: switch to bdrv_dirty_bitmap_merge_internal
  iotests: add test for bitmap mirror
  mirror: move some checks to QMP

John Snow (2):
  drive-mirror: add support for sync=bitmap mode=never
  drive-mirror: add support for conditional and always bitmap sync modes

 include/block/block_int.h   |    4 +-
 block/mirror.c              |   96 +-
 blockdev.c                  |   71 +-
 tests/test-block-iothread.c |    4 +-
 qapi/block-core.json        |   29 +-
 tests/qemu-iotests/284      |  547 +++++++
 tests/qemu-iotests/284.out  | 2846 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/group    |    1 +
 8 files changed, 3567 insertions(+), 31 deletions(-)
 create mode 100755 tests/qemu-iotests/284
 create mode 100644 tests/qemu-iotests/284.out

-- 
2.20.1



Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by no-reply@patchew.org 4 years, 3 months ago
Patchew URL: https://patchew.org/QEMU/20200218100740.2228521-1-f.gruenbichler@proxmox.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Message-id: 20200218100740.2228521-1-f.gruenbichler@proxmox.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200218094402.26625-1-philmd@redhat.com -> patchew/20200218094402.26625-1-philmd@redhat.com
 * [new tag]         patchew/20200218100740.2228521-1-f.gruenbichler@proxmox.com -> patchew/20200218100740.2228521-1-f.gruenbichler@proxmox.com
Switched to a new branch 'test'
50da7dd mirror: move some checks to QMP
2236985 iotests: add test for bitmap mirror
b65aa31 mirror: switch to bdrv_dirty_bitmap_merge_internal
c03e05f mirror: add check for bitmap-mode without bitmap
1e909f2 drive-mirror: add support for conditional and always bitmap sync modes
e7baad3 drive-mirror: add support for sync=bitmap mode=never

=== OUTPUT BEGIN ===
1/6 Checking commit e7baad3a765b (drive-mirror: add support for sync=bitmap mode=never)
2/6 Checking commit 1e909f289147 (drive-mirror: add support for conditional and always bitmap sync modes)
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 48 lines checked

Patch 2/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

3/6 Checking commit c03e05f9f733 (mirror: add check for bitmap-mode without bitmap)
4/6 Checking commit b65aa312e64e (mirror: switch to bdrv_dirty_bitmap_merge_internal)
5/6 Checking commit 2236985625be (iotests: add test for bitmap mirror)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#24: 
new file mode 100755

total: 0 errors, 1 warnings, 3397 lines checked

Patch 5/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/6 Checking commit 50da7ddd187c (mirror: move some checks to QMP)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200218100740.2228521-1-f.gruenbichler@proxmox.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by John Snow 4 years, 2 months ago

On 2/18/20 5:07 AM, Fabian Grünbichler wrote:
> picking up on John's in-progress patch series from last summer, this is
> a stab at rebasing and adding test cases for the low-hanging fruits:
> 
> - bitmap mirror mode with always/on-success/never bitmap sync mode
> - incremental mirror mode as sugar for bitmap + on-success
> 
> Fabian Grünbichler (4):
>   mirror: add check for bitmap-mode without bitmap
>   mirror: switch to bdrv_dirty_bitmap_merge_internal
>   iotests: add test for bitmap mirror
>   mirror: move some checks to QMP
> 
> John Snow (2):
>   drive-mirror: add support for sync=bitmap mode=never
>   drive-mirror: add support for conditional and always bitmap sync modes
> 
>  include/block/block_int.h   |    4 +-
>  block/mirror.c              |   96 +-
>  blockdev.c                  |   71 +-
>  tests/test-block-iothread.c |    4 +-
>  qapi/block-core.json        |   29 +-
>  tests/qemu-iotests/284      |  547 +++++++
>  tests/qemu-iotests/284.out  | 2846 +++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group    |    1 +
>  8 files changed, 3567 insertions(+), 31 deletions(-)
>  create mode 100755 tests/qemu-iotests/284
>  create mode 100644 tests/qemu-iotests/284.out
> 

Hi Fabian! Thanks for picking this up. I'm a bit behind on my mail, but
this on my list to look at.

(Hint to other maintainers: It might be a while.)

--js


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Fabian Grünbichler 4 years, 1 month ago
On February 25, 2020 10:54 pm, John Snow wrote:
> On 2/18/20 5:07 AM, Fabian Grünbichler wrote:
>> picking up on John's in-progress patch series from last summer, this is
>> a stab at rebasing and adding test cases for the low-hanging fruits:
>> 
>> - bitmap mirror mode with always/on-success/never bitmap sync mode
>> - incremental mirror mode as sugar for bitmap + on-success
>> 
>> Fabian Grünbichler (4):
>>   mirror: add check for bitmap-mode without bitmap
>>   mirror: switch to bdrv_dirty_bitmap_merge_internal
>>   iotests: add test for bitmap mirror
>>   mirror: move some checks to QMP
>> 
>> John Snow (2):
>>   drive-mirror: add support for sync=bitmap mode=never
>>   drive-mirror: add support for conditional and always bitmap sync modes
>> 
>>  include/block/block_int.h   |    4 +-
>>  block/mirror.c              |   96 +-
>>  blockdev.c                  |   71 +-
>>  tests/test-block-iothread.c |    4 +-
>>  qapi/block-core.json        |   29 +-
>>  tests/qemu-iotests/284      |  547 +++++++
>>  tests/qemu-iotests/284.out  | 2846 +++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/group    |    1 +
>>  8 files changed, 3567 insertions(+), 31 deletions(-)
>>  create mode 100755 tests/qemu-iotests/284
>>  create mode 100644 tests/qemu-iotests/284.out
>> 
> 
> Hi Fabian! Thanks for picking this up. I'm a bit behind on my mail, but
> this on my list to look at.
> 
> (Hint to other maintainers: It might be a while.)
> 
> --js

Hi,

ping? ;)

(it would be great to get some feedback as we'd like to ship this 
backported to 4.2 or 5.0 with our next point release)

kind regards,
Fabian


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Max Reitz 3 years, 9 months ago
On 18.02.20 11:07, Fabian Grünbichler wrote:

[Sorry :/]

> picking up on John's in-progress patch series from last summer, this is
> a stab at rebasing and adding test cases for the low-hanging fruits:
> 
> - bitmap mirror mode with always/on-success/never bitmap sync mode
> - incremental mirror mode as sugar for bitmap + on-success
> 
> Fabian Grünbichler (4):
>   mirror: add check for bitmap-mode without bitmap
>   mirror: switch to bdrv_dirty_bitmap_merge_internal
>   iotests: add test for bitmap mirror
>   mirror: move some checks to QMP
> 
> John Snow (2):
>   drive-mirror: add support for sync=bitmap mode=never
>   drive-mirror: add support for conditional and always bitmap sync modes

Looks reasonable to me.  I would indeed merge patches 2 through 4 into a
single one, and perhaps switch patches 5 and 6.

Also, we still need an S-o-b from John on patch 2.

I have one question: When the mirror job completes successfully (or is
cancelled “successfully”), the bitmap is always fully cleared when the
job completes, right?  (Unless in “never” mode.)

Not that I think we should change the current implementation of “clear
sync_bitmap; merge dirty_bitmap into sync_bitmap;”.  Just a question for
understanding.


Soo...  What’s the plan?

Max

Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by John Snow 3 years, 8 months ago
On 8/21/20 9:03 AM, Max Reitz wrote:
> On 18.02.20 11:07, Fabian Grünbichler wrote:
> 
> [Sorry :/]
> 
>> picking up on John's in-progress patch series from last summer, this is
>> a stab at rebasing and adding test cases for the low-hanging fruits:
>>
>> - bitmap mirror mode with always/on-success/never bitmap sync mode
>> - incremental mirror mode as sugar for bitmap + on-success
>>
>> Fabian Grünbichler (4):
>>    mirror: add check for bitmap-mode without bitmap
>>    mirror: switch to bdrv_dirty_bitmap_merge_internal
>>    iotests: add test for bitmap mirror
>>    mirror: move some checks to QMP
>>
>> John Snow (2):
>>    drive-mirror: add support for sync=bitmap mode=never
>>    drive-mirror: add support for conditional and always bitmap sync modes
> 
> Looks reasonable to me.  I would indeed merge patches 2 through 4 into a
> single one, and perhaps switch patches 5 and 6.
> 
> Also, we still need an S-o-b from John on patch 2.
> 

Whoops! Will do.

> I have one question: When the mirror job completes successfully (or is
> cancelled “successfully”), the bitmap is always fully cleared when the
> job completes, right?  (Unless in “never” mode.)
> 

I don't remember personally, it's been a minute ...

> Not that I think we should change the current implementation of “clear
> sync_bitmap; merge dirty_bitmap into sync_bitmap;”.  Just a question for
> understanding.
> 
> 
> Soo...  What’s the plan?
> 
> Max
> 


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Fabian Grünbichler 3 years, 8 months ago
On August 21, 2020 3:03 pm, Max Reitz wrote:
> On 18.02.20 11:07, Fabian Grünbichler wrote:
> 
> [Sorry :/]

same, I've been meaning to ping/pick this back up but other stuff got in 
the way. so thanks for the reminder to get this upstream ;)

> 
>> picking up on John's in-progress patch series from last summer, this is
>> a stab at rebasing and adding test cases for the low-hanging fruits:
>> 
>> - bitmap mirror mode with always/on-success/never bitmap sync mode
>> - incremental mirror mode as sugar for bitmap + on-success
>> 
>> Fabian Grünbichler (4):
>>   mirror: add check for bitmap-mode without bitmap
>>   mirror: switch to bdrv_dirty_bitmap_merge_internal
>>   iotests: add test for bitmap mirror
>>   mirror: move some checks to QMP
>> 
>> John Snow (2):
>>   drive-mirror: add support for sync=bitmap mode=never
>>   drive-mirror: add support for conditional and always bitmap sync modes
> 
> Looks reasonable to me.  I would indeed merge patches 2 through 4 into a
> single one, and perhaps switch patches 5 and 6.
> 
> Also, we still need an S-o-b from John on patch 2.
> 
> I have one question: When the mirror job completes successfully (or is
> cancelled “successfully”), the bitmap is always fully cleared when the
> job completes, right?  (Unless in “never” mode.)

I have to take a closer look as well, it's been a while ;) IIRC the idea 
was that failed mirrors would allow re-using the bitmap for a next 
attempt, unless the mode is always. we are not using that feature (yet) 
though (see below).

> Not that I think we should change the current implementation of “clear
> sync_bitmap; merge dirty_bitmap into sync_bitmap;”.  Just a question for
> understanding.
> 
> 
> Soo...  What’s the plan?

I'll rebase, squash as suggested and resend next week! I am not sure how 
the S-O-B by John is supposed to enter the mix - should I just include 
it in the squashed patch (which would be partly authored, but 
not-yet-signed-off by him otherwise?)? do you pick it up once he's 
replied with one?

FWIW, with been running with this for quite a while downstream with no 
issues, but we are only using the following part:

- create bitmap(s)
- (incrementally) replicate storage volume(s) out of band (using ZFS)
- incrementally drive mirror as part of a live migration of VM
- drop bitmap(s)

so no fancy semi-permanent bitmap that gets re-used here. I've been 
toying with implementing some sort of generic replication feature akin 
to zfs send/recv though, but given that we only have built-in persistent 
bitmaps with qcow2 and the chance of some other tool or the user messing 
up other image formats is high, the safe usage scenarios are a bit 
limited.

we do use such long-running bitmaps for our new backup driver though, 
and it works quite well there!


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Max Reitz 3 years, 8 months ago
On 03.09.20 12:13, Fabian Grünbichler wrote:
> On August 21, 2020 3:03 pm, Max Reitz wrote:
>> On 18.02.20 11:07, Fabian Grünbichler wrote:
>>
>> [Sorry :/]
> 
> same, I've been meaning to ping/pick this back up but other stuff got in 
> the way. so thanks for the reminder to get this upstream ;)
> 
>>
>>> picking up on John's in-progress patch series from last summer, this is
>>> a stab at rebasing and adding test cases for the low-hanging fruits:
>>>
>>> - bitmap mirror mode with always/on-success/never bitmap sync mode
>>> - incremental mirror mode as sugar for bitmap + on-success
>>>
>>> Fabian Grünbichler (4):
>>>   mirror: add check for bitmap-mode without bitmap
>>>   mirror: switch to bdrv_dirty_bitmap_merge_internal
>>>   iotests: add test for bitmap mirror
>>>   mirror: move some checks to QMP
>>>
>>> John Snow (2):
>>>   drive-mirror: add support for sync=bitmap mode=never
>>>   drive-mirror: add support for conditional and always bitmap sync modes
>>
>> Looks reasonable to me.  I would indeed merge patches 2 through 4 into a
>> single one, and perhaps switch patches 5 and 6.
>>
>> Also, we still need an S-o-b from John on patch 2.
>>
>> I have one question: When the mirror job completes successfully (or is
>> cancelled “successfully”), the bitmap is always fully cleared when the
>> job completes, right?  (Unless in “never” mode.)
> 
> I have to take a closer look as well, it's been a while ;)

No problem, I’m... *cough* not exactly in a hurry.

> IIRC the idea 
> was that failed mirrors would allow re-using the bitmap for a next 
> attempt, unless the mode is always. we are not using that feature (yet) 
> though (see below).
> 
>> Not that I think we should change the current implementation of “clear
>> sync_bitmap; merge dirty_bitmap into sync_bitmap;”.  Just a question for
>> understanding.
>>
>>
>> Soo...  What’s the plan?
> 
> I'll rebase, squash as suggested and resend next week!

OK :)

> I am not sure how 
> the S-O-B by John is supposed to enter the mix - should I just include 
> it in the squashed patch (which would be partly authored, but 
> not-yet-signed-off by him otherwise?)?

I’m not too sure on the proceedings, actually.  I think it should be
fine if you put his S-o-b there, as long as your patch is somehow based
on a patch that he sent earlier with his S-o-b underneath.  But I’m not
sure.

> do you pick it up once he's replied with one?

Yes, that’s what would be best.

> FWIW, with been running with this for quite a while downstream with no 
> issues, but we are only using the following part:
> 
> - create bitmap(s)
> - (incrementally) replicate storage volume(s) out of band (using ZFS)
> - incrementally drive mirror as part of a live migration of VM
> - drop bitmap(s)
> 
> so no fancy semi-permanent bitmap that gets re-used here. I've been 
> toying with implementing some sort of generic replication feature akin 
> to zfs send/recv though, but given that we only have built-in persistent 
> bitmaps with qcow2 and the chance of some other tool or the user messing 
> up other image formats is high, the safe usage scenarios are a bit 
> limited.

OK.

> we do use such long-running bitmaps for our new backup driver though, 
> and it works quite well there!

Good! :)

Max

Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Kevin Wolf 3 years, 8 months ago
Am 03.09.2020 um 13:04 hat Max Reitz geschrieben:
> On 03.09.20 12:13, Fabian Grünbichler wrote:
> > On August 21, 2020 3:03 pm, Max Reitz wrote:
> >> On 18.02.20 11:07, Fabian Grünbichler wrote:
> > I am not sure how 
> > the S-O-B by John is supposed to enter the mix - should I just include 
> > it in the squashed patch (which would be partly authored, but 
> > not-yet-signed-off by him otherwise?)?
> 
> I’m not too sure on the proceedings, actually.  I think it should be
> fine if you put his S-o-b there, as long as your patch is somehow based
> on a patch that he sent earlier with his S-o-b underneath.  But I’m not
> sure.

Signed-off-by means that John certifies the DCO for the patch (at least
the original version that you possibly modified), so you cannot just add
it without asking him.

John should reply with a Signed-off-by line to the patch in question.
Then you (Fabian) can add it in the next version of the series (if I
understand correctly, you're going to respin anyway).

I see that patch 2 doesn't have any S-o-b at all. It should have both
John's and Fabian's.

Kevin
Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Max Reitz 3 years, 8 months ago
On 03.09.20 14:38, Kevin Wolf wrote:
> Am 03.09.2020 um 13:04 hat Max Reitz geschrieben:
>> On 03.09.20 12:13, Fabian Grünbichler wrote:
>>> On August 21, 2020 3:03 pm, Max Reitz wrote:
>>>> On 18.02.20 11:07, Fabian Grünbichler wrote:
>>> I am not sure how 
>>> the S-O-B by John is supposed to enter the mix - should I just include 
>>> it in the squashed patch (which would be partly authored, but 
>>> not-yet-signed-off by him otherwise?)?
>>
>> I’m not too sure on the proceedings, actually.  I think it should be
>> fine if you put his S-o-b there, as long as your patch is somehow based
>> on a patch that he sent earlier with his S-o-b underneath.  But I’m not
>> sure.
> 
> Signed-off-by means that John certifies the DCO for the patch (at least
> the original version that you possibly modified), so you cannot just add
> it without asking him.

But what if you take a patch from someone and heavily modify it –
wouldn’t you keep the original S-o-b and explain the modifications in
the commit message?

Max

> John should reply with a Signed-off-by line to the patch in question.
> Then you (Fabian) can add it in the next version of the series (if I
> understand correctly, you're going to respin anyway).
> 
> I see that patch 2 doesn't have any S-o-b at all. It should have both
> John's and Fabian's.
> 
> Kevin
> 


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Kevin Wolf 3 years, 8 months ago
Am 03.09.2020 um 14:57 hat Max Reitz geschrieben:
> On 03.09.20 14:38, Kevin Wolf wrote:
> > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben:
> >> On 03.09.20 12:13, Fabian Grünbichler wrote:
> >>> On August 21, 2020 3:03 pm, Max Reitz wrote:
> >>>> On 18.02.20 11:07, Fabian Grünbichler wrote:
> >>> I am not sure how 
> >>> the S-O-B by John is supposed to enter the mix - should I just include 
> >>> it in the squashed patch (which would be partly authored, but 
> >>> not-yet-signed-off by him otherwise?)?
> >>
> >> I’m not too sure on the proceedings, actually.  I think it should be
> >> fine if you put his S-o-b there, as long as your patch is somehow based
> >> on a patch that he sent earlier with his S-o-b underneath.  But I’m not
> >> sure.
> > 
> > Signed-off-by means that John certifies the DCO for the patch (at least
> > the original version that you possibly modified), so you cannot just add
> > it without asking him.
> 
> But what if you take a patch from someone and heavily modify it –
> wouldn’t you keep the original S-o-b and explain the modifications in
> the commit message?

Ah, if that patch already had a S-o-b, then yes. You keep it not only to
show who touched the patch, but also because your own S-o-b depends on
the one from the original author (you only have the rights to contribute
it because the original author had them and could pass them on to you).

I thought it was based on a patch that came without S-o-b.

Kevin
Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Fabian Grünbichler 3 years, 8 months ago
On September 3, 2020 3:23 pm, Kevin Wolf wrote:
> Am 03.09.2020 um 14:57 hat Max Reitz geschrieben:
>> On 03.09.20 14:38, Kevin Wolf wrote:
>> > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben:
>> >> On 03.09.20 12:13, Fabian Grünbichler wrote:
>> >>> On August 21, 2020 3:03 pm, Max Reitz wrote:
>> >>>> On 18.02.20 11:07, Fabian Grünbichler wrote:
>> >>> I am not sure how 
>> >>> the S-O-B by John is supposed to enter the mix - should I just include 
>> >>> it in the squashed patch (which would be partly authored, but 
>> >>> not-yet-signed-off by him otherwise?)?
>> >>
>> >> I’m not too sure on the proceedings, actually.  I think it should be
>> >> fine if you put his S-o-b there, as long as your patch is somehow based
>> >> on a patch that he sent earlier with his S-o-b underneath.  But I’m not
>> >> sure.
>> > 
>> > Signed-off-by means that John certifies the DCO for the patch (at least
>> > the original version that you possibly modified), so you cannot just add
>> > it without asking him.
>> 
>> But what if you take a patch from someone and heavily modify it –
>> wouldn’t you keep the original S-o-b and explain the modifications in
>> the commit message?
> 
> Ah, if that patch already had a S-o-b, then yes. You keep it not only to
> show who touched the patch, but also because your own S-o-b depends on
> the one from the original author (you only have the rights to contribute
> it because the original author had them and could pass them on to you).
> 
> I thought it was based on a patch that came without S-o-b.

it is (taken from John's git, with his approval, and implicit admission 
that S-O-B is just missing because it was a WIP branch/tree that I 
started from). that was also the reason why I kept that patch unmodified 
and sent my modifications as patches on-top, to make it easier for John 
to verify that that one patch is his original WIP one and add this 
missing S-O-B ;)


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Kevin Wolf 3 years, 8 months ago
Am 03.09.2020 um 15:36 hat Fabian Grünbichler geschrieben:
> On September 3, 2020 3:23 pm, Kevin Wolf wrote:
> > Am 03.09.2020 um 14:57 hat Max Reitz geschrieben:
> >> On 03.09.20 14:38, Kevin Wolf wrote:
> >> > Am 03.09.2020 um 13:04 hat Max Reitz geschrieben:
> >> >> On 03.09.20 12:13, Fabian Grünbichler wrote:
> >> >>> On August 21, 2020 3:03 pm, Max Reitz wrote:
> >> >>>> On 18.02.20 11:07, Fabian Grünbichler wrote:
> >> >>> I am not sure how 
> >> >>> the S-O-B by John is supposed to enter the mix - should I just include 
> >> >>> it in the squashed patch (which would be partly authored, but 
> >> >>> not-yet-signed-off by him otherwise?)?
> >> >>
> >> >> I’m not too sure on the proceedings, actually.  I think it should be
> >> >> fine if you put his S-o-b there, as long as your patch is somehow based
> >> >> on a patch that he sent earlier with his S-o-b underneath.  But I’m not
> >> >> sure.
> >> > 
> >> > Signed-off-by means that John certifies the DCO for the patch (at least
> >> > the original version that you possibly modified), so you cannot just add
> >> > it without asking him.
> >> 
> >> But what if you take a patch from someone and heavily modify it –
> >> wouldn’t you keep the original S-o-b and explain the modifications in
> >> the commit message?
> > 
> > Ah, if that patch already had a S-o-b, then yes. You keep it not only to
> > show who touched the patch, but also because your own S-o-b depends on
> > the one from the original author (you only have the rights to contribute
> > it because the original author had them and could pass them on to you).
> > 
> > I thought it was based on a patch that came without S-o-b.
> 
> it is (taken from John's git, with his approval, and implicit admission 
> that S-O-B is just missing because it was a WIP branch/tree that I 
> started from). that was also the reason why I kept that patch unmodified 
> and sent my modifications as patches on-top, to make it easier for John 
> to verify that that one patch is his original WIP one and add this 
> missing S-O-B ;)

Yeah, then John should just reply to the patch and add the S-o-b.

Complications like this are why I always use 'git commit -s' and have it
already in my development branches rather than only adding it when
preparing the patch emails to send.

Kevin


Re: [RFC qemu 0/6] mirror: implement incremental and bitmap modes
Posted by Max Reitz 3 years, 8 months ago
On 03.09.20 15:36, Fabian Grünbichler wrote:
> On September 3, 2020 3:23 pm, Kevin Wolf wrote:
>> Am 03.09.2020 um 14:57 hat Max Reitz geschrieben:
>>> On 03.09.20 14:38, Kevin Wolf wrote:
>>>> Am 03.09.2020 um 13:04 hat Max Reitz geschrieben:
>>>>> On 03.09.20 12:13, Fabian Grünbichler wrote:
>>>>>> On August 21, 2020 3:03 pm, Max Reitz wrote:
>>>>>>> On 18.02.20 11:07, Fabian Grünbichler wrote:
>>>>>> I am not sure how 
>>>>>> the S-O-B by John is supposed to enter the mix - should I just include 
>>>>>> it in the squashed patch (which would be partly authored, but 
>>>>>> not-yet-signed-off by him otherwise?)?
>>>>>
>>>>> I’m not too sure on the proceedings, actually.  I think it should be
>>>>> fine if you put his S-o-b there, as long as your patch is somehow based
>>>>> on a patch that he sent earlier with his S-o-b underneath.  But I’m not
>>>>> sure.
>>>>
>>>> Signed-off-by means that John certifies the DCO for the patch (at least
>>>> the original version that you possibly modified), so you cannot just add
>>>> it without asking him.
>>>
>>> But what if you take a patch from someone and heavily modify it –
>>> wouldn’t you keep the original S-o-b and explain the modifications in
>>> the commit message?
>>
>> Ah, if that patch already had a S-o-b, then yes. You keep it not only to
>> show who touched the patch, but also because your own S-o-b depends on
>> the one from the original author (you only have the rights to contribute
>> it because the original author had them and could pass them on to you).
>>
>> I thought it was based on a patch that came without S-o-b.
> 
> it is (taken from John's git, with his approval, and implicit admission 
> that S-O-B is just missing because it was a WIP branch/tree that I 
> started from). that was also the reason why I kept that patch unmodified 
> and sent my modifications as patches on-top, to make it easier for John 
> to verify that that one patch is his original WIP one and add this 
> missing S-O-B ;)

OK, I see now.  I thought the S-o-b got lost somewhere, but was present
originally.

Max