Re: [PATCH 1/4] block/quorum.c: stable children names

[Alberto Garcia <berto@igalia.com>]

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