| Message ID | ce10f8cf2bb9ae8a1505b59bbc2199f7b4966990.1574356137.git.lukasstraub2@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | colo: Introduce resource agent and high-level test | expand |
On 11/21/19 11:49 AM, Lukas Straub wrote: > If we remove the child with the highest index from the quorum, > decrement s->next_child_index. This way we get stable children > names as long as we only remove the last child. > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > --- > block/quorum.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > 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? > + if (child_id == s->next_child_index - 1) { > + s->next_child_index--; > + } > + > bdrv_drained_begin(bs); > > /* We can safely remove this child now */ > -- > 2.20.1 > >
On Thu, 21 Nov 2019 12:04:58 -0600 Eric Blake <eblake@redhat.com> wrote: > On 11/21/19 11:49 AM, Lukas Straub wrote: > > If we remove the child with the highest index from the quorum, > > decrement s->next_child_index. This way we get stable children > > names as long as we only remove the last child. > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > > --- > > block/quorum.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > 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? Hi, 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. > > + if (child_id == s->next_child_index - 1) { > > + s->next_child_index--; > > + } > > + > > bdrv_drained_begin(bs); > > > > /* We can safely remove this child now */ > > -- > > 2.20.1 > > > > >
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
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
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); + if (child_id == s->next_child_index - 1) { + s->next_child_index--; + } + bdrv_drained_begin(bs); /* We can safely remove this child now */
If we remove the child with the highest index from the quorum, decrement s->next_child_index. This way we get stable children names as long as we only remove the last child. Signed-off-by: Lukas Straub <lukasstraub2@web.de> --- block/quorum.c | 6 ++++++ 1 file changed, 6 insertions(+) -- 2.20.1