Re: [PATCH v9 09/21] jobs: use job locks also in the unit tests

[Vladimir Sementsov-Ogievskiy]

On 7/6/22 23:15, Emanuele Giuseppe Esposito wrote:
> Add missing job synchronization in the unit tests, with
> explicit locks.
>
> We are deliberately using _locked functions wrapped by a guard
> instead of a normal call because the normal call will be removed
> in future, as the only usage is limited to the tests.
>
> In other words, if a function like job_pause() is/will be only used
> in tests to avoid:
>
> WITH_JOB_LOCK_GUARD(){
>      job_pause_locked();
> }
>
> then it is not worth keeping job_pause(), and just use the guard.

I'm not sure. Why not worth keeping simpler code in tests? But I don't insist.

> Note: at this stage, job_{lock/unlock} and job lock guard macros
> are *nop*.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[..]

> --- a/tests/unit/test-blockjob-txn.c
> +++ b/tests/unit/test-blockjob-txn.c
> @@ -116,8 +116,10 @@ static void test_single_job(int expected)
>       job = test_block_job_start(1, true, expected, &result, txn);
>       job_start(&job->job);
>   
> -    if (expected == -ECANCELED) {
> -        job_cancel(&job->job, false);
> +    WITH_JOB_LOCK_GUARD() {
> +        if (expected == -ECANCELED) {
> +            job_cancel_locked(&job->job, false);
> +        }
>       }

"if (expected == ..)" may be kept around LOCK_GUARD section.

>   
>       while (result == -EINPROGRESS) {
> @@ -160,13 +162,15 @@ static void test_pair_jobs(int expected1, int expected2)
>       /* Release our reference now to trigger as many nice
>        * use-after-free bugs as possible.
>        */
> -    job_txn_unref(txn);

[..]

>       cancel_common(s);
>   }
>   
> +/*
> + * This test always runs in the main loop, so there is no
> + * need to protect job->status.
> + */

That made me ask:

1. Are all tests always run in main loop? If yes, why to protect status reading 
in test_complete_in_standby() ?

2. Maybe, we don't need to protect anything here? Why to protect other things 
if we run everything in main loop?





--
Best regards,
Vladimir