[3/3] block: change rollback sequence in qmp_transaction

Submitted by Wayne Xia on April 1, 2013, 10:01 a.m.

Details

Message ID 1364810491-21404-4-git-send-email-xiawenc@linux.vnet.ibm.com
State New
Headers show

Commit Message

Wayne Xia April 1, 2013, 10:01 a.m.
Last operaton should be cancelled first.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Eric Blake April 1, 2013, 3:52 p.m.
On 04/01/2013 04:01 AM, Wenchao Xia wrote:
>   Last operaton should be cancelled first.

s/operaton/operation/

[I don't care enough about US vs. UK to say whether canceled or
cancelled looks better]

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 75416fb..a24d10e 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>          dev_entry = dev_entry->next;
>  
>          states = g_malloc0(sizeof(BlkTransactionStates));
> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
> +        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);

Is this a bug fix that for something that can be triggered by existing
use of the 'transaction' command even without the additions you made in
patches 1 and 2?  If so, this probably ought to come first in the
series, and you probably ought to consider enhancing the testsuite to
show why it matters.
Wayne Xia April 2, 2013, 2:34 a.m.
于 2013-4-1 23:52, Eric Blake 写道:
> On 04/01/2013 04:01 AM, Wenchao Xia wrote:
>>    Last operaton should be cancelled first.
>
> s/operaton/operation/
>
> [I don't care enough about US vs. UK to say whether canceled or
> cancelled looks better]
>
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 75416fb..a24d10e 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -954,7 +954,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
>>           dev_entry = dev_entry->next;
>>
>>           states = g_malloc0(sizeof(BlkTransactionStates));
>> -        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
>> +        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);
>
> Is this a bug fix that for something that can be triggered by existing
> use of the 'transaction' command even without the additions you made in
> patches 1 and 2?  If so, this probably ought to come first in the
> series, and you probably ought to consider enhancing the testsuite to
> show why it matters.
>
   Originally it only support backing chain, it will have no
difference about the sequence to delete it, but matters if
other types of operation are added.
Kevin Wolf April 2, 2013, 1:59 p.m.
Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>   Last operaton should be cancelled first.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Should it? This commit message does little to convince me.

Kevin
Wayne Xia April 3, 2013, 5:35 a.m.
于 2013-4-2 21:59, Kevin Wolf 写道:
> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>    Last operaton should be cancelled first.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Should it? This commit message does little to convince me.
>
> Kevin
>
   I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
"snapshot_blkdev_internal ide0 snap0", then in rollback delete of
snap0 would be wrong.
Kevin Wolf April 3, 2013, 9:03 a.m.
Am 03.04.2013 um 07:35 hat Wenchao Xia geschrieben:
> 于 2013-4-2 21:59, Kevin Wolf 写道:
> >Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
> >>   Last operaton should be cancelled first.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >
> >Should it? This commit message does little to convince me.
> >
> >Kevin
> >
>   I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
> "snapshot_blkdev_internal ide0 snap0", then in rollback delete of
> snap0 would be wrong.

I think this problem exists only because you didn't properly separate
the prepare and the commit stage. If any changes only took effect in a
non-failing commit stage, then you don't ever have to delete a snapshot.
Instead, you just abort the creation of the snapshot.

Kevin
Wayne Xia April 3, 2013, 10:35 a.m.
于 2013-4-3 17:03, Kevin Wolf 写道:
> Am 03.04.2013 um 07:35 hat Wenchao Xia geschrieben:
>> 于 2013-4-2 21:59, Kevin Wolf 写道:
>>> Am 01.04.2013 um 12:01 hat Wenchao Xia geschrieben:
>>>>    Last operaton should be cancelled first.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>
>>> Should it? This commit message does little to convince me.
>>>
>>> Kevin
>>>
>>    I think so, if two request are "snapshot_blkdev ide0 newimage.qcow2",
>> "snapshot_blkdev_internal ide0 snap0", then in rollback delete of
>> snap0 would be wrong.
>
> I think this problem exists only because you didn't properly separate
> the prepare and the commit stage. If any changes only took effect in a
> non-failing commit stage, then you don't ever have to delete a snapshot.
> Instead, you just abort the creation of the snapshot.
>
> Kevin
>
   OK, I got your point, will split bdrv_snapshot_create() before adding
it in qmp_transaction.

Patch hide | download patch | download mbox

diff --git a/blockdev.c b/blockdev.c
index 75416fb..a24d10e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -954,7 +954,7 @@  void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
         dev_entry = dev_entry->next;
 
         states = g_malloc0(sizeof(BlkTransactionStates));
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+        QSIMPLEQ_INSERT_HEAD(&snap_bdrv_states, states, entry);
 
         states->action = dev_info;
         switch (dev_info->kind) {