On Tue, 26 Nov 2019 15:21:37 +0100
Alberto Garcia <be...@igalia.com> wrote:
> On Thu 21 Nov 2019 07:34:45 PM CET, Lukas Straub wrote:
> >> > diff --git a/block/quorum.c b/block/quorum.c
> >> > index df68adcfaa..6100d4108a 100644
> >> > --- a/block/quorum.c
> >> > +++ b/block/quorum.c
> >> > @@ -1054,6 +1054,12 @@ static void quorum_del_child(BlockDriverState
> >> > *bs, BdrvChild *child,
> >> > /* We know now that num_children > threshold, so blkverify must be
> >> > false */
> >> > assert(!s->is_blkverify);
> >> >
> >> > + unsigned child_id;
> >> > + sscanf(child->name, "children.%u", &child_id);
> >>
> >> sscanf() cannot detect overflow. Do we trust our input enough to
> >> ignore this shortfall in the interface, or should we be using saner
> >> interfaces like qemu_strtoul()? For that matter, why do we have to
> >> reparse something; is it not already available somewhere in numerical
> >> form?
> >
> > Yes, I wondered about that too, but found no other way. But the input
> > is trusted, AFAIK the only way to add child nodes is trough
> > quorum_add_child above and quorum_open and there already are adequate
> > checks there.
>
> I also don't see any other way to get that value, unless we change
> BDRVQuorumState to store that information (e.g. instead of children
> being a list of pointers BdrvChild ** it could be a list of {pointer,
> index}, or something like that).
>
> There's another (more convoluted) alternative if we don't want to parse
> child->name. Since we only want to know if the child number equals
> s->next_child_index - 1, we can do it the other way around:
>
> snprintf(str, 32, "children.%u", s->next_child_index - 1);
>
> and then compare str and child->name.
>
> Berto
Hi,
I will do it your way, then it's also more consistent with the name
creation in quorum_add and quorum_open.
Regards,
Lukas Straub
