Re: [Qemu-devel] [PULL 21/31] block: Avoid bs->blk in bdrv_next()

[Kevin Wolf <kwolf@redhat.com>]

Am 20.05.2016 um 10:05 hat Kevin Wolf geschrieben:
> Am 20.05.2016 um 09:54 hat Paolo Bonzini geschrieben:
> > > +/* Iterates over all top-level BlockDriverStates, i.e. BDSs that are owned by
> > > + * the monitor or attached to a BlockBackend */
> > > +BdrvNextIterator *bdrv_next(BdrvNextIterator *it, BlockDriverState **bs)
> > > +{
> > > +    if (!it) {
> > > +        it = g_new(BdrvNextIterator, 1);
> > 
> > Hmm, who frees it?  (Especially if the caller exits the loop
> > prematurely, which means you cannot just free it before returning NULL).
> >  I think it's better to:
> > 
> > - allocate the iterator on the stack and make bdrv_next return a BDS *
> > 
> > - and add a bdrv_first function that does this:
> > 
> > > +        *it = (BdrvNextIterator) {
> > > +            .phase = BDRV_NEXT_BACKEND_ROOTS,
> > > +        };
> > 
> > and then returns bdrv_next(it);
> > 
> > - if desirable add a macro that abstracts the calls to bdrv_first and
> > bdrv_next.
> 
> Already posted a fix. I chose to keep the interface and free the
> BdrvNextIterator inside bdrv_next(), when we return NULL after the last
> element.

Oops, should have actually read your email... You're right about callers
that prematurely exit the loop, of course.

I still don't really like first/next interfaces, though. Perhaps start
the iteration with bs == NULL instead of it == NULL?

Kevin