Re: [Qemu-devel] [PATCH v2 6/6] tests/block-job-txn: Don't start block job before adding to txn

[Fam Zheng]

On Fri, 04/07 14:05, John Snow wrote:
> 
> 
> On 04/07/2017 09:28 AM, Stefan Hajnoczi wrote:
> > On Fri, Apr 07, 2017 at 02:54:14PM +0800, Fam Zheng wrote:
> >> Previously, before test_block_job_start returns, the job can already
> >> complete, as a result, the transactional state of other jobs added to
> >> the same txn later cannot be handled correctly.
> >>
> >> Move the block_job_start() calls to callers after
> >> block_job_txn_add_job() calls.
> >>
> >> Signed-off-by: Fam Zheng 
> >> ---
> >>  tests/test-blockjob-txn.c | 6 +-
> >>  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > CCing John Snow because he looked at block jobs completing during txn
> > setup recently.
> > 
> > Stefan
> > 
> 
> This matches the changes we made to qmp_transaction, but I forgot to (or
> didn't take care to)  change the qtest as it didn't cause a regression
> at the time.
> 
> I wonder if I should make it a runtime error to add a job to a
> transaction which has already "started" to make sure that this interface
> is not misused, as this test highlights that there were still some
> remaining "bad" uses of the interface.
> 
> Regardless...
> 
> Thanks for the CC. ACK

So, shall we merge this for 2.9?

Fam

Re: [Qemu-devel] [PATCH v2 6/6] tests/block-job-txn: Don't start block job before adding to txn

[John Snow]

On 04/07/2017 09:28 AM, Stefan Hajnoczi wrote:
> On Fri, Apr 07, 2017 at 02:54:14PM +0800, Fam Zheng wrote:
>> Previously, before test_block_job_start returns, the job can already
>> complete, as a result, the transactional state of other jobs added to
>> the same txn later cannot be handled correctly.
>>
>> Move the block_job_start() calls to callers after
>> block_job_txn_add_job() calls.
>>
>> Signed-off-by: Fam Zheng 
>> ---
>>  tests/test-blockjob-txn.c | 6 +-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> CCing John Snow because he looked at block jobs completing during txn
> setup recently.
> 
> Stefan
> 

This matches the changes we made to qmp_transaction, but I forgot to (or
didn't take care to)  change the qtest as it didn't cause a regression
at the time.

I wonder if I should make it a runtime error to add a job to a
transaction which has already "started" to make sure that this interface
is not misused, as this test highlights that there were still some
remaining "bad" uses of the interface.

Regardless...

Thanks for the CC. ACK

Re: [Qemu-devel] [PATCH v2 6/6] tests/block-job-txn: Don't start block job before adding to txn

[Stefan Hajnoczi]

On Fri, Apr 07, 2017 at 02:54:14PM +0800, Fam Zheng wrote:
> Previously, before test_block_job_start returns, the job can already
> complete, as a result, the transactional state of other jobs added to
> the same txn later cannot be handled correctly.
> 
> Move the block_job_start() calls to callers after
> block_job_txn_add_job() calls.
> 
> Signed-off-by: Fam Zheng 
> ---
>  tests/test-blockjob-txn.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

CCing John Snow because he looked at block jobs completing during txn
setup recently.

Stefan


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v2 6/6] tests/block-job-txn: Don't start block job before adding to txn

[Fam Zheng]

Previously, before test_block_job_start returns, the job can already
complete, as a result, the transactional state of other jobs added to
the same txn later cannot be handled correctly.

Move the block_job_start() calls to callers after
block_job_txn_add_job() calls.

Signed-off-by: Fam Zheng 
---
 tests/test-blockjob-txn.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 4ccbda1..0f80194 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -110,7 +110,6 @@ static BlockJob *test_block_job_start(unsigned int 
iterations,
 s->result = result;
 data->job = s;
 data->result = result;
-block_job_start(>common);
 return >common;
 }
 
@@ -123,6 +122,7 @@ static void test_single_job(int expected)
 txn = block_job_txn_new();
 job = test_block_job_start(1, true, expected, );
 block_job_txn_add_job(txn, job);
+block_job_start(job);
 
 if (expected == -ECANCELED) {
 block_job_cancel(job);
@@ -164,6 +164,8 @@ static void test_pair_jobs(int expected1, int expected2)
 block_job_txn_add_job(txn, job1);
 job2 = test_block_job_start(2, true, expected2, );
 block_job_txn_add_job(txn, job2);
+block_job_start(job1);
+block_job_start(job2);
 
 if (expected1 == -ECANCELED) {
 block_job_cancel(job1);
@@ -223,6 +225,8 @@ static void test_pair_jobs_fail_cancel_race(void)
 block_job_txn_add_job(txn, job1);
 job2 = test_block_job_start(2, false, 0, );
 block_job_txn_add_job(txn, job2);
+block_job_start(job1);
+block_job_start(job2);
 
 block_job_cancel(job1);
 
-- 
2.9.3