[PATCH V2] vector: return false if realloc fails in vector_alloc_slot func

[PATCH V2] vector: return false if realloc fails in vector_alloc_slot func

[Zhiqiang Liu]

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 <liuzhiqiang26@huawei.com>
Signed-off-by: lixiaokeng <lixiaokeng@huawei.com>
---
 libmultipath/config.c       |  2 +-
 libmultipath/foreign.c      |  2 +-
 libmultipath/foreign/nvme.c |  6 +++---
 libmultipath/vector.c       | 22 +++++++++-------------
 libmultipath/vector.h       |  6 ++++--
 5 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index 49723add..4f5cefda 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -131,7 +131,7 @@ find_hwe (const struct _vector *hwtable,
 	vector_foreach_slot_backwards (hwtable, tmp, i) {
 		if (hwe_regmatch(tmp, vendor, product, revision))
 			continue;
-		if (vector_alloc_slot(result) != NULL) {
+		if (vector_alloc_slot(result)) {
 			vector_set_slot(result, tmp);
 			n++;
 		}
diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
index 26f92672..c2335ed5 100644
--- a/libmultipath/foreign.c
+++ b/libmultipath/foreign.c
@@ -236,7 +236,7 @@ static int _init_foreign(const char *multipath_dir, const char *const enable)
 			goto dl_err;
 		}

-		if (vector_alloc_slot(foreigns) == NULL) {
+		if (!vector_alloc_slot(foreigns)) {
 			goto dl_err;
 		}

diff --git a/libmultipath/foreign/nvme.c b/libmultipath/foreign/nvme.c
index da85e515..88b6aee2 100644
--- a/libmultipath/foreign/nvme.c
+++ b/libmultipath/foreign/nvme.c
@@ -731,12 +731,12 @@ static void _find_controllers(struct context *ctx, struct nvme_map *map)
 		test_ana_support(map, path->ctl);

 		path->pg.gen.ops = &nvme_pg_ops;
-		if (vector_alloc_slot(&path->pg.pathvec) == NULL) {
+		if (!vector_alloc_slot(&path->pg.pathvec)) {
 			cleanup_nvme_path(path);
 			continue;
 		}
 		vector_set_slot(&path->pg.pathvec, path);
-		if (vector_alloc_slot(&map->pgvec) == NULL) {
+		if (!vector_alloc_slot(&map->pgvec)) {
 			cleanup_nvme_path(path);
 			continue;
 		}
@@ -792,7 +792,7 @@ static int _add_map(struct context *ctx, struct udev_device *ud,
 	map->subsys = subsys;
 	map->gen.ops = &nvme_map_ops;

-	if (vector_alloc_slot(ctx->mpvec) == NULL) {
+	if (!vector_alloc_slot(ctx->mpvec)) {
 		cleanup_nvme_map(map);
 		return FOREIGN_ERR;
 	}
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));
 	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;
+	return true;
 }

 int
@@ -203,7 +199,7 @@ int vector_find_or_add_slot(vector v, void *value)

 	if (n >= 0)
 		return n;
-	if (vector_alloc_slot(v) == NULL)
+	if (!vector_alloc_slot(v))
 		return -1;
 	vector_set_slot(v, value);
 	return VECTOR_SIZE(v) - 1;
diff --git a/libmultipath/vector.h b/libmultipath/vector.h
index e16ec461..cb64b7d6 100644
--- a/libmultipath/vector.h
+++ b/libmultipath/vector.h
@@ -23,6 +23,8 @@
 #ifndef _VECTOR_H
 #define _VECTOR_H

+#include <stdbool.h>
+
 /* vector definition */
 struct _vector {
 	int allocated;
@@ -60,7 +62,7 @@ typedef struct _vector *vector;
 			__t = vector_alloc();				\
 		if (__t != NULL) {					\
 			vector_foreach_slot(__v, __j, __i) {		\
-				if (vector_alloc_slot(__t) == NULL) {	\
+				if (!vector_alloc_slot(__t)) {	\
 					vector_free(__t);		\
 					__t = NULL;			\
 					break;				\
@@ -73,7 +75,7 @@ typedef struct _vector *vector;

 /* Prototypes */
 extern vector vector_alloc(void);
-extern void *vector_alloc_slot(vector v);
+extern bool vector_alloc_slot(vector v);
 vector vector_reset(vector v);
 extern void vector_free(vector v);
 #define vector_free_const(x) vector_free((vector)(long)(x))
-- 
2.24.0.windows.2

Re: [PATCH V2] vector: return false if realloc fails in vector_alloc_slot func

[Martin Wilck]

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 <liuzhiqiang26@huawei.com>
> Signed-off-by: lixiaokeng <lixiaokeng@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

Re: [PATCH V2] vector: return false if realloc fails in vector_alloc_slot func

[Zhiqiang Liu]

On 2020/8/11 17:46, Martin Wilck wrote:
> On Tue, 2020-08-11 at 11:23 +0800, Zhiqiang Liu wrote:
> 
>> 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.
Thanks for your reply.
I will introduce new var to shorten this line and init new slot with
using loop in the v3 patch.

Thanks again.

Regards.
Zhiqiang Liu

> 
>>  	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
> 
> 
> 
> .
>