> > > > +typedef struct ThrottleGroup {
> > > > + char *name; /* This is constant during the lifetime of the group */
> > >
> > > Is this also protected by throttle_groups_lock?
> > >
> > > I guess throttle_groups_lock must be held in order to read this
> > > field - otherwise there is a risk that the object is freed
> > > unless you have already incremented the refcount.
> >
> > The creation and destruction of ThrottleGroup objects are
> > protected by throttle_groups_lock. That includes handling the
> > memory for that field and its contents.
> >
> > Once the ThrottleGroup is created the 'name' field doesn't need
> > any additional locking since it remains constant during the
> > lifetime of the group.
> >
> > The only way to read it from the outside is
> > throttle_group_get_name() and that's safe (until you release the
> > reference to the group, that is).
>
> Right, the race condition is when the group is released.
>
> Looking at this again, the assumption isn't that
> throttle_groups_lock is held. The AioContext lock is held by
> throttle_group_get_name() users and that's why there is no race when
> releasing the reference.
>
> If someone adds a throttle_group_get_name() caller in the future
> without holding AioContext, then we'd be in trouble. That is why
> documenting the locking constraints is useful.
But that would only happen if you access bs->throttle_state without
holding bs's AioContext. I understand that it's implicit that you
should hold the context there.
Maybe I can update the throttle_group_* API to use BlockDriverState
in all cases instead of ThrottleState, it's probably more clear like
that.
Berto
