Re: [Qemu-devel] [PATCH] iotests: fix wait_until_completed()

[Stefan Hajnoczi]

On Wed, Apr 04, 2018 at 09:42:41AM +0800, Peter Xu wrote:
> On Tue, Apr 03, 2018 at 01:59:18PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Mar 27, 2018 at 10:21:55AM +0800, Peter Xu wrote:
> > > On Mon, Mar 26, 2018 at 12:47:39PM +0200, Kevin Wolf wrote:
> > > > Am 26.03.2018 um 08:11 hat Peter Xu geschrieben:
> > > The patch fixes the other case when there
> > > are two events: one JOB_COMPLETED plus another (e.g., RESUME) event.
> > > When that happens, logically we should return one JOB_COMPLETED event,
> > > but the old code will return the other event (e.g., RESUME).
> > > 
> > > > 
> > > > Wouldn't it be much easier to just add a 'break'?
> > > 
> > > Yes, it's the same.  But IMHO those logics (e.g., the completed
> > > variable) are not really needed at all.  This one is simpler.
> > 
> > No, the outer loop is needed so that the function waits until
> > BLOCK_JOB_COMPLETED is received.  It's not possible to do it with a
> > single for loop.
> 
> Indeed.  But then I would still slightly prefer removing the
> "completed" var:
> 
>     def wait_until_completed(self, drive='drive0', check_offset=True):
>         '''Wait for a block job to finish, returning the event'''
>         while True:
>             for event in self.vm.get_qmp_events(wait=True):
>                 if event['event'] == 'BLOCK_JOB_COMPLETED':
>                     self.assert_qmp(event, 'data/device', drive)
>                     self.assert_qmp_absent(event, 'data/error')
>                     if check_offset:
>                         self.assert_qmp(event, 'data/offset', 
> event['data']['len'])
>                     self.assert_no_active_block_jobs()
>                     return event
> 
> Or a single break would work too.  Do either of you have any
> preference?  I can repost in either way.  Thanks,

Looks good to me!

Stefan

signature.asc
Description: PGP signature