On Tue, 2025-05-06 at 18:01 -0400, Benjamin Marzinski wrote: > On Mon, May 05, 2025 at 06:29:56PM +0200, Martin Wilck wrote: > > Try to silence a gcc warning. Also, replace the wrong-looking > > VECTOR_DEFAULT_SIZE by 1 (after all, we've just deleted a single > > element). > > > > Found by Fedora's static analysis [1]. > > > > [1] > > https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html > > > > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx> > > --- > > libmpathutil/vector.c | 16 ++++++++-------- > > 1 file changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/libmpathutil/vector.c b/libmpathutil/vector.c > > index 7f763cb..3386651 100644 > > --- a/libmpathutil/vector.c > > +++ b/libmpathutil/vector.c > > @@ -107,28 +107,28 @@ int find_slot(vector v, const void *addr) > > void > > vector_del_slot(vector v, int slot) > > { > > - int i; > > + int i, allocated; > > > > if (!v || !v->allocated || slot < 0 || slot >= > > VECTOR_SIZE(v)) > > return; > > > > for (i = slot + 1; i < VECTOR_SIZE(v); i++) > > - v->slot[i-1] = v->slot[i]; > > + v->slot[i - 1] = v->slot[i]; > > > > - v->allocated -= VECTOR_DEFAULT_SIZE; > > + allocated = v->allocated - 1; > > > > - if (v->allocated <= 0) { > > + if (allocated <= 0) { > > free(v->slot); > > v->slot = NULL; > > v->allocated = 0; > > } else { > > void *new_slot; > > > > - new_slot = realloc(v->slot, sizeof (void *) * v- > > >allocated); > > - if (!new_slot) > > - v->allocated += VECTOR_DEFAULT_SIZE; > > - else > > + new_slot = realloc(v->slot, sizeof(void *) * > > allocated); > > + if (new_slot) { > > v->slot = new_slot; > > + v->allocated = allocated; > > + } > > Actually, now that I think about this a bit more, I don't think that > this was ever the right thing to do. If the realloc() fails, I see no > harm in keeping the smaller v->allocated value. Future realloc()s and > free()s will still work correctly. On the other hand, if we restore > the > old, larger v->allocated value (or with your code, never reduce it in > the first place) there will be a duplicate value on the list, which I > can definitely see causing problems. This is a fundamental problem of our vector implementation, IMO. We don't distinguish between "allocated" and "used", or IOW, we use the term "allocated" while we mean "used". My thinking was that "allocated" should track the actual size of the allocated memory. But you are right, it's better to have the C library track the allocated size and keep in mind that v->allocated actually has the semantics of "used". I'll send a v2 with this change. Regards Martin PS: I've wanted to improve the vector implementation for years. IMO we should separate "allocated" and "used", and should cease reallocating the vectors whenever an element is added or removed, which is horribly inefficient. I'd actually started working on that a while ago but didn't finish. Anyway, that's future material.
