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
