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

From: Alberto Garcia <berto@igalia.com>
To: Lukas Straub <lukasstraub2@web.de>, Eric Blake <eblake@redhat.com>
Cc: "Zhang, Chen" <chen.zhang@intel.com>,
	Jason Wang <jasowang@redhat.com>,
	qemu-devel <qemu-devel@nongnu.org>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH 1/4] block/quorum.c: stable children names
Date: Tue, 26 Nov 2019 15:21:37 +0100	[thread overview]
Message-ID: <w51imn667m6.fsf@maestria.local.igalia.com> (raw)
In-Reply-To: <20191121193445.2361b846@luklap>

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