Clearing .cancelled before leaving the main loop when the job has been
soft-cancelled is no longer necessary since job_is_cancelled() only
returns true for jobs that have been force-cancelled.

Therefore, this only makes a differences in places that call
job_cancel_requested().  In block/mirror.c, this is done only before
.cancelled was cleared.

In job.c, there are two callers:
- job_completed_txn_abort() asserts that .cancelled is true, so keeping
  it true will not affect this place.

- job_complete() refuses to let a job complete that has .cancelled set.
  It is correct to refuse to let the user invoke job-complete on mirror
  jobs that have already been soft-cancelled.

With this change, there are no places that reset .cancelled to false and
so we can be sure that .force_cancel can only be true of .cancelled is
true as well.  Assert this in job_is_cancelled().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/mirror.c | 2 --
 job.c          | 4 +++-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index af89c1716a..f94aa52fae 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -939,7 +939,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
         while (!job_cancel_requested(&s->common.job) && !s->should_complete) {
             job_yield(&s->common.job);
         }
-        s->common.job.cancelled = false;
         goto immediate_exit;
     }
 
@@ -1078,7 +1077,6 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
              * completion.
              */
             assert(QLIST_EMPTY(&bs->tracked_requests));
-            s->common.job.cancelled = false;
             need_drain = false;
             break;
         }
diff --git a/job.c b/job.c
index 2bd3c946a7..2ce6865ab2 100644
--- a/job.c
+++ b/job.c
@@ -217,7 +217,9 @@ const char *job_type_str(const Job *job)
 
 bool job_is_cancelled(Job *job)
 {
-    return job->cancelled && job->force_cancel;
+    /* force_cancel may be true only if cancelled is true, too */
+    assert(job->cancelled || !job->force_cancel);
+    return job->force_cancel;
 }
 
 bool job_cancel_requested(Job *job)
-- 
2.31.1

On Fri, Aug 06, 2021 at 11:38:58AM +0200, Max Reitz wrote:
> Clearing .cancelled before leaving the main loop when the job has been
> soft-cancelled is no longer necessary since job_is_cancelled() only
> returns true for jobs that have been force-cancelled.
> 
> Therefore, this only makes a differences in places that call
> job_cancel_requested().  In block/mirror.c, this is done only before
> .cancelled was cleared.
> 
> In job.c, there are two callers:
> - job_completed_txn_abort() asserts that .cancelled is true, so keeping
>   it true will not affect this place.
> 
> - job_complete() refuses to let a job complete that has .cancelled set.
>   It is correct to refuse to let the user invoke job-complete on mirror
>   jobs that have already been soft-cancelled.
> 
> With this change, there are no places that reset .cancelled to false and
> so we can be sure that .force_cancel can only be true of .cancelled is

s/of/if/

> true as well.  Assert this in job_is_cancelled().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 2 --
>  job.c          | 4 +++-
>  2 files changed, 3 insertions(+), 3 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

06.08.2021 12:38, Max Reitz wrote:
> Clearing .cancelled before leaving the main loop when the job has been
> soft-cancelled is no longer necessary since job_is_cancelled() only
> returns true for jobs that have been force-cancelled.
> 
> Therefore, this only makes a differences in places that call
> job_cancel_requested().  In block/mirror.c, this is done only before
> .cancelled was cleared.
> 
> In job.c, there are two callers:
> - job_completed_txn_abort() asserts that .cancelled is true, so keeping
>    it true will not affect this place.
> 
> - job_complete() refuses to let a job complete that has .cancelled set.
>    It is correct to refuse to let the user invoke job-complete on mirror
>    jobs that have already been soft-cancelled.
> 
> With this change, there are no places that reset .cancelled to false and
> so we can be sure that .force_cancel can only be true of .cancelled is
> true as well.  Assert this in job_is_cancelled().
> 
> Signed-off-by: Max Reitz<mreitz@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir