[PULL,19/32] commit: Fix use of error handling policy

Commit Message

Kevin Wolf July 8, 2016, 5:21 p.m. UTC

Commit implemented the 'enospc' policy as 'ignore' if the error was not
ENOSPC. The QAPI documentation promises that it's treated as 'stop'.
Using the common block job error handling function fixes this and also
adds the missing QMP event.

Signed-off-by: Kevin Wolf <[e-mail]>
Reviewed-by: Max Reitz <[e-mail]>
---
 block/commit.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Eric Blake July 8, 2016, 9:36 p.m. UTC | #1

On 07/08/2016 11:21 AM, Kevin Wolf wrote:
> Commit implemented the 'enospc' policy as 'ignore' if the error was not

Was there supposed to be a commit id in this sentence?

> ENOSPC. The QAPI documentation promises that it's treated as 'stop'.
> Using the common block job error handling function fixes this and also
> adds the missing QMP event.
> 
> Signed-off-by: Kevin Wolf <[e-mail]>
> Reviewed-by: Max Reitz <[e-mail]>
> ---

Kevin Wolf July 11, 2016, 11:22 a.m. UTC | #2

Am 08.07.2016 um 23:36 hat Eric Blake geschrieben:
> On 07/08/2016 11:21 AM, Kevin Wolf wrote:
> > Commit implemented the 'enospc' policy as 'ignore' if the error was not
> 
> Was there supposed to be a commit id in this sentence?

Yes. Or well, I intended to put a commit id there, but it's really since
live commit was first merged in 747ff602.

Anyway, too late (I pulled Max's tree, so I can't rebase any more), and
it's easy enough to figure this out with git blame.

Kevin

Paolo Bonzini July 11, 2016, 11:40 a.m. UTC | #3

On 11/07/2016 13:22, Kevin Wolf wrote:
>>> > > Commit implemented the 'enospc' policy as 'ignore' if the error was not
>> > 
>> > Was there supposed to be a commit id in this sentence?
> Yes. Or well, I intended to put a commit id there, but it's really since
> live commit was first merged in 747ff602.

Curious, I read it as "[The commit block job] implemented the 'enospc'
policy as 'ignore'"!

Paolo

> Anyway, too late (I pulled Max's tree, so I can't rebase any more), and
> it's easy enough to figure this out with git blame.

Max Reitz July 11, 2016, 11:57 a.m. UTC | #4

On 11.07.2016 13:22, Kevin Wolf wrote:
> Am 08.07.2016 um 23:36 hat Eric Blake geschrieben:
>> On 07/08/2016 11:21 AM, Kevin Wolf wrote:
>>> Commit implemented the 'enospc' policy as 'ignore' if the error was not
>>
>> Was there supposed to be a commit id in this sentence?
> 
> Yes. Or well, I intended to put a commit id there, but it's really since
> live commit was first merged in 747ff602.

Well, I read it the same as Paolo, so I think it can very well stay as
it is.

> Anyway, too late (I pulled Max's tree, so I can't rebase any more),

Don't say that. :-P

Max

>                                                                     and
> it's easy enough to figure this out with git blame.
> 
> Kevin
>

Kevin Wolf July 11, 2016, 12:37 p.m. UTC | #5

Am 11.07.2016 um 13:40 hat Paolo Bonzini geschrieben:
> 
> 
> On 11/07/2016 13:22, Kevin Wolf wrote:
> >>> > > Commit implemented the 'enospc' policy as 'ignore' if the error was not
> >> > 
> >> > Was there supposed to be a commit id in this sentence?
> > Yes. Or well, I intended to put a commit id there, but it's really since
> > live commit was first merged in 747ff602.
> 
> Curious, I read it as "[The commit block job] implemented the 'enospc'
> policy as 'ignore'"!

Ah, yes, I guess that's what I meant then. Makes a lot more sense than
referring to a commit when the bug has always existed. There was a full
weekend between writing this and replying to Eric, if that works as an
excuse for misunderstanding my own commit message. ;-)

Kevin

Patch

diff --git a/block/commit.c b/block/commit.c
index 8b534d7..5d11eb6 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -171,9 +171,9 @@  wait:
             bytes_written += n * BDRV_SECTOR_SIZE;
         }
         if (ret < 0) {
-            if (s->on_error == BLOCKDEV_ON_ERROR_STOP ||
-                s->on_error == BLOCKDEV_ON_ERROR_REPORT||
-                (s->on_error == BLOCKDEV_ON_ERROR_ENOSPC && ret == -ENOSPC)) {
+            BlockErrorAction action =
+                block_job_error_action(&s->common, false, s->on_error, -ret);
+            if (action == BLOCK_ERROR_ACTION_REPORT) {
                 goto out;
             } else {
                 n = 0;