Re: [Qemu-devel] [PATCH v2 6/6] tests/block-job-txn: Don't start block job before adding to txn
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
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
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
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(&s->common); return &s->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, &result); 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, &result2); 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, &result2); block_job_txn_add_job(txn, job2); +block_job_start(job1); +block_job_start(job2); block_job_cancel(job1); -- 2.9.3