On Tue, 2020-08-11 at 11:23 +0800, Zhiqiang Liu wrote:
> In vector_alloc_slot func, if REALLOC fails, it means new slot
> allocation fails. However, it just update v->allocated and then
> return the old v->slot without new slot. So, the caller will take
> the last old slot as the new allocated slot, and use it by calling
> vector_set_slot func. Finally, the data of last slot is lost.
>
> Here, we rewrite vector_alloc_slot as suggested by Martin Wilck:
> - increment v->allocated only after successful allocation,
> - avoid the "if (v->slot)" conditional by just calling realloc(),
> - make sure all newly allocated vector elements are set to NULL,
> - change return value to bool.
>
> Signed-off-by: Zhiqiang Liu <liuzhiqian...@huawei.com>
> Signed-off-by: lixiaokeng <lixiaok...@huawei.com>
Thanks a lot, this is very nice. We're almost there (see below).
> diff --git a/libmultipath/vector.c b/libmultipath/vector.c
> index 501cf4c5..39e2c20f 100644
> --- a/libmultipath/vector.c
> +++ b/libmultipath/vector.c
> @@ -35,26 +35,22 @@ vector_alloc(void)
> }
>
> /* allocated one slot */
> -void *
> +bool
> vector_alloc_slot(vector v)
> {
> void *new_slot = NULL;
>
> if (!v)
> - return NULL;
> -
> - v->allocated += VECTOR_DEFAULT_SIZE;
> - if (v->slot)
> - new_slot = REALLOC(v->slot, sizeof (void *) * v-
> >allocated);
> - else
> - new_slot = (void *) MALLOC(sizeof (void *) * v-
> >allocated);
> + return false;
>
> + new_slot = REALLOC(v->slot, sizeof (void *) * (v->allocated +
> VECTOR_DEFAULT_SIZE));
Please wrap this line. We basically still use a 80-columns limit, with
the exception of string constants (mostly condlog() lines). We don't
strictly enforce it, but 92 columns is a bit too much for my taste.
> if (!new_slot)
> - v->allocated -= VECTOR_DEFAULT_SIZE;
> - else
> - v->slot = new_slot;
> + return false;
>
> - return v->slot;
> + v->slot = new_slot;
> + v->allocated += VECTOR_DEFAULT_SIZE;
> + v->slot[VECTOR_SIZE(v) - 1] = NULL;
This assumes that VECTOR_DEFAULT_SIZE == 1. While that has been true
essentially forever, there's no guarantee for the future. Better use a
for loop, or memset().
Regards
Martin
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
