Re: [PATCH v2 4/4] iotests: Mirror with different source/target size

[Kevin Wolf]

Am 13.05.2020 um 13:17 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 11.05.2020 16:58, Kevin Wolf wrote:
> > This tests that the mirror job catches situations where the target node
> > has a different size than the source node. It must also forbid resize
> > operations when the job is already running.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > Message-Id: <address@hidden>
> > Reviewed-by: Eric Blake <address@hidden>
> > Signed-off-by: Kevin Wolf <address@hidden>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> > ---
> >   tests/qemu-iotests/041     | 45 ++++++++++++++++++++++++++++++++++++++
> >   tests/qemu-iotests/041.out |  4 ++--
> >   2 files changed, 47 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> > index 1812dd8479..601c756117 100755
> > --- a/tests/qemu-iotests/041
> > +++ b/tests/qemu-iotests/041
> > @@ -240,6 +240,49 @@ class TestSingleBlockdev(TestSingleDrive):
> 
> 
> Hmm, probably resize tests would be good in the TestSingleDrive, to
> cover drive-mirror too. Still, there shouldn't be any difference.. So,
> I don't think it worth doing.

At first, I tried to do a single implementation that works for both, but
it wasn't easily possible. I'm not sure any more if it was the only
problem, but at least drive-mirror targets don't have a known node-name,
so we would have to guess it from query-named-block-nodes. Not
impossible, but maybe not worth it...

> >                                target=self.qmp_target)
> >           self.assert_qmp(result, 'error/class', 'GenericError')
> > +    def do_test_resize(self, device, node):
> > +        def pre_finalize():
> 
> [..]
> 
> > +    def do_test_target_size(self, size):
> > +        result = self.vm.qmp('block_resize', node_name=self.qmp_target,
> > +                             size=size)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        result = self.vm.qmp(self.qmp_cmd, job_id='job0',
> > +                             device='drive0', sync='full', 
> > auto_dismiss=False,
> > +                             target=self.qmp_target)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        result = self.vm.run_job('job0')
> > +        self.assertEqual(result, 'Source and target image have different 
> > sizes')
> 
> Hmm, and this proves, that we are not very good with handling early
> errors. A lot better would be to fail earlier, on blockdev-mirror
> command. And, as shown by your previous series, backup works better in
> this case. But again, it's for another series.

At least it shows that errp does make it to the client. :-)

I'm not sure which way is currently better. QMP handlers are still
synchronous code that blocks the rest of QEMU, so doing things only
inside the coroutine does have some benefits. Maybe with coroutine QMP
handlers this will become less of a concern, though.

Kevin