| Message ID | 1364810491-21404-4-git-send-email-xiawenc@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.
于 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.
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
于 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.
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
于 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.
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) {
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(-)