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

From: Lukas Straub <lukasstraub2@web.de>
To: Alberto Garcia <berto@igalia.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: Wed, 27 Nov 2019 22:20:24 +0100	[thread overview]
Message-ID: <20191127222024.4bcb34b8@luklap> (raw)
In-Reply-To: <w51imn667m6.fsf@maestria.local.igalia.com>

On Tue, 26 Nov 2019 15:21:37 +0100
Alberto Garcia <berto@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