[PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[jmoyer]

From: Jeff Moyer <jmoyer@redhat.com>

Static analysis points out that the cast of bus->major and bus->minor
to a signed type in the call to parent_dev_path could result in a
negative number.  I sincerely doubt we'll see major and minor numbers
that large, but let's fix it.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 ndctl/lib/libndctl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ddbdd9a..f75dbd4 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -710,11 +710,12 @@ NDCTL_EXPORT void ndctl_set_log_priority(struct ndctl_ctx *ctx, int priority)
 	daxctl_set_log_priority(ctx->daxctl_ctx, priority);
 }
 
-static char *__dev_path(char *type, int major, int minor, int parent)
+static char *__dev_path(char *type, unsigned int major, unsigned int minor,
+			int parent)
 {
 	char *path, *dev_path;
 
-	if (asprintf(&path, "/sys/dev/%s/%d:%d%s", type, major, minor,
+	if (asprintf(&path, "/sys/dev/%s/%u:%u%s", type, major, minor,
 				parent ? "/device" : "") < 0)
 		return NULL;
 
@@ -723,7 +724,7 @@ static char *__dev_path(char *type, int major, int minor, int parent)
 	return dev_path;
 }
 
-static char *parent_dev_path(char *type, int major, int minor)
+static char *parent_dev_path(char *type, unsigned int major, unsigned int minor)
 {
         return __dev_path(type, major, minor, 1);
 }
-- 
2.43.0

Re: [PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[Verma, Vishal L]

On Fri, 2024-05-03 at 16:54 -0400, jmoyer@redhat.com wrote:
> From: Jeff Moyer <jmoyer@redhat.com>
> 
> Static analysis points out that the cast of bus->major and bus->minor
> to a signed type in the call to parent_dev_path could result in a
> negative number.  I sincerely doubt we'll see major and minor numbers
> that large, but let's fix it.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
>  ndctl/lib/libndctl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ddbdd9a..f75dbd4 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -710,11 +710,12 @@ NDCTL_EXPORT void ndctl_set_log_priority(struct
> ndctl_ctx *ctx, int priority)
>  	daxctl_set_log_priority(ctx->daxctl_ctx, priority);
>  }
>  
> -static char *__dev_path(char *type, int major, int minor, int
> parent)
> +static char *__dev_path(char *type, unsigned int major, unsigned int
> minor,
> +			int parent)
>  {
>  	char *path, *dev_path;
>  
> -	if (asprintf(&path, "/sys/dev/%s/%d:%d%s", type, major,
> minor,
> +	if (asprintf(&path, "/sys/dev/%s/%u:%u%s", type, major,
> minor,
>  				parent ? "/device" : "") < 0)
>  		return NULL;
>  
> @@ -723,7 +724,7 @@ static char *__dev_path(char *type, int major,
> int minor, int parent)
>  	return dev_path;
>  }
>  
> -static char *parent_dev_path(char *type, int major, int minor)
> +static char *parent_dev_path(char *type, unsigned int major,
> unsigned int minor)
>  {
>          return __dev_path(type, major, minor, 1);
>  }

address some open scan hub warnings

[jmoyer]

These issues are pretty minor, but probably worth fixing.

[PATCH ndctl 1/2] ndctl/keys.c: don't leak fd in error cases
[PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[PATCH ndctl 1/2] ndctl/keys.c: don't leak fd in error cases

[jmoyer]

From: Jeff Moyer <jmoyer@redhat.com>

Static analysis points out that fd is leaked in some cases.  The
change to the while loop is optional.  I only did that to make the
code consistent.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 ndctl/keys.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/ndctl/keys.c b/ndctl/keys.c
index 2c1f474..cc55204 100644
--- a/ndctl/keys.c
+++ b/ndctl/keys.c
@@ -108,7 +108,7 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
 	struct stat st;
 	ssize_t read_bytes = 0;
 	int rc, fd;
-	char *blob, *pl, *rdptr;
+	char *blob = NULL, *pl, *rdptr;
 	char prefix[] = "load ";
 	bool need_prefix = false;
 
@@ -125,16 +125,16 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
 	rc = fstat(fd, &st);
 	if (rc < 0) {
 		fprintf(stderr, "stat: %s\n", strerror(errno));
-		return NULL;
+		goto out_close;
 	}
 	if ((st.st_mode & S_IFMT) != S_IFREG) {
 		fprintf(stderr, "%s not a regular file\n", path);
-		return NULL;
+		goto out_close;
 	}
 
 	if (st.st_size == 0 || st.st_size > 4096) {
 		fprintf(stderr, "Invalid blob file size\n");
-		return NULL;
+		goto out_close;
 	}
 
 	*size = st.st_size;
@@ -166,15 +166,13 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
 			fprintf(stderr, "Failed to read from blob file: %s\n",
 					strerror(errno));
 			free(blob);
-			close(fd);
-			return NULL;
+			blob = NULL;
+			goto out_close;
 		}
 		read_bytes += rc;
 		rdptr += rc;
 	} while (read_bytes != st.st_size);
 
-	close(fd);
-
 	if (postfix) {
 		pl += read_bytes;
 		*pl = ' ';
@@ -182,6 +180,8 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
 		rc = sprintf(pl, "keyhandle=%s", postfix);
 	}
 
+out_close:
+	close(fd);
 	return blob;
 }
 
-- 
2.43.5

[PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[jmoyer]

From: Jeff Moyer <jmoyer@redhat.com>

Static analysis points out that the cast of bus->major and bus->minor
to a signed type in the call to parent_dev_path could result in a
negative number.  I sincerely doubt we'll see major and minor numbers
that large, but let's fix it.

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
---
 ndctl/lib/libndctl.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
index ddbdd9a..f75dbd4 100644
--- a/ndctl/lib/libndctl.c
+++ b/ndctl/lib/libndctl.c
@@ -710,11 +710,12 @@ NDCTL_EXPORT void ndctl_set_log_priority(struct ndctl_ctx *ctx, int priority)
 	daxctl_set_log_priority(ctx->daxctl_ctx, priority);
 }
 
-static char *__dev_path(char *type, int major, int minor, int parent)
+static char *__dev_path(char *type, unsigned int major, unsigned int minor,
+			int parent)
 {
 	char *path, *dev_path;
 
-	if (asprintf(&path, "/sys/dev/%s/%d:%d%s", type, major, minor,
+	if (asprintf(&path, "/sys/dev/%s/%u:%u%s", type, major, minor,
 				parent ? "/device" : "") < 0)
 		return NULL;
 
@@ -723,7 +724,7 @@ static char *__dev_path(char *type, int major, int minor, int parent)
 	return dev_path;
 }
 
-static char *parent_dev_path(char *type, int major, int minor)
+static char *parent_dev_path(char *type, unsigned int major, unsigned int minor)
 {
         return __dev_path(type, major, minor, 1);
 }
-- 
2.43.5

Re: [PATCH ndctl 1/2] ndctl/keys.c: don't leak fd in error cases

[Dave Jiang]

On 8/20/24 11:26 AM, jmoyer@redhat.com wrote:
> From: Jeff Moyer <jmoyer@redhat.com>
> 
> Static analysis points out that fd is leaked in some cases.  The
> change to the while loop is optional.  I only did that to make the
> code consistent.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Reviewed-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  ndctl/keys.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ndctl/keys.c b/ndctl/keys.c
> index 2c1f474..cc55204 100644
> --- a/ndctl/keys.c
> +++ b/ndctl/keys.c
> @@ -108,7 +108,7 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
>  	struct stat st;
>  	ssize_t read_bytes = 0;
>  	int rc, fd;
> -	char *blob, *pl, *rdptr;
> +	char *blob = NULL, *pl, *rdptr;
>  	char prefix[] = "load ";
>  	bool need_prefix = false;
>  
> @@ -125,16 +125,16 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
>  	rc = fstat(fd, &st);
>  	if (rc < 0) {
>  		fprintf(stderr, "stat: %s\n", strerror(errno));
> -		return NULL;
> +		goto out_close;
>  	}
>  	if ((st.st_mode & S_IFMT) != S_IFREG) {
>  		fprintf(stderr, "%s not a regular file\n", path);
> -		return NULL;
> +		goto out_close;
>  	}
>  
>  	if (st.st_size == 0 || st.st_size > 4096) {
>  		fprintf(stderr, "Invalid blob file size\n");
> -		return NULL;
> +		goto out_close;
>  	}
>  
>  	*size = st.st_size;
> @@ -166,15 +166,13 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
>  			fprintf(stderr, "Failed to read from blob file: %s\n",
>  					strerror(errno));
>  			free(blob);
> -			close(fd);
> -			return NULL;
> +			blob = NULL;
> +			goto out_close;
>  		}
>  		read_bytes += rc;
>  		rdptr += rc;
>  	} while (read_bytes != st.st_size);
>  
> -	close(fd);
> -
>  	if (postfix) {
>  		pl += read_bytes;
>  		*pl = ' ';
> @@ -182,6 +180,8 @@ char *ndctl_load_key_blob(const char *path, int *size, const char *postfix,
>  		rc = sprintf(pl, "keyhandle=%s", postfix);
>  	}
>  
> +out_close:
> +	close(fd);
>  	return blob;
>  }
>  

Re: [PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[Dave Jiang]

On 8/20/24 11:26 AM, jmoyer@redhat.com wrote:
> From: Jeff Moyer <jmoyer@redhat.com>
> 
> Static analysis points out that the cast of bus->major and bus->minor
> to a signed type in the call to parent_dev_path could result in a
> negative number.  I sincerely doubt we'll see major and minor numbers
> that large, but let's fix it.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Reviewed-by: Dave Jiang <dave.jiang@intel.com>

> ---
>  ndctl/lib/libndctl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ddbdd9a..f75dbd4 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -710,11 +710,12 @@ NDCTL_EXPORT void ndctl_set_log_priority(struct ndctl_ctx *ctx, int priority)
>  	daxctl_set_log_priority(ctx->daxctl_ctx, priority);
>  }
>  
> -static char *__dev_path(char *type, int major, int minor, int parent)
> +static char *__dev_path(char *type, unsigned int major, unsigned int minor,
> +			int parent)
>  {
>  	char *path, *dev_path;
>  
> -	if (asprintf(&path, "/sys/dev/%s/%d:%d%s", type, major, minor,
> +	if (asprintf(&path, "/sys/dev/%s/%u:%u%s", type, major, minor,
>  				parent ? "/device" : "") < 0)
>  		return NULL;
>  
> @@ -723,7 +724,7 @@ static char *__dev_path(char *type, int major, int minor, int parent)
>  	return dev_path;
>  }
>  
> -static char *parent_dev_path(char *type, int major, int minor)
> +static char *parent_dev_path(char *type, unsigned int major, unsigned int minor)
>  {
>          return __dev_path(type, major, minor, 1);
>  }

Re: [PATCH ndctl 1/2] ndctl/keys.c: don't leak fd in error cases

[Verma, Vishal L]

On Tue, 2024-08-20 at 14:26 -0400, jmoyer@redhat.com wrote:
> From: Jeff Moyer <jmoyer@redhat.com>
> 
> Static analysis points out that fd is leaked in some cases.  The
> change to the while loop is optional.  I only did that to make the
> code consistent.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
>  ndctl/keys.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/ndctl/keys.c b/ndctl/keys.c
> index 2c1f474..cc55204 100644
> --- a/ndctl/keys.c
> +++ b/ndctl/keys.c
> @@ -108,7 +108,7 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  	struct stat st;
>  	ssize_t read_bytes = 0;
>  	int rc, fd;
> -	char *blob, *pl, *rdptr;
> +	char *blob = NULL, *pl, *rdptr;
>  	char prefix[] = "load ";
>  	bool need_prefix = false;
>  
> @@ -125,16 +125,16 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  	rc = fstat(fd, &st);
>  	if (rc < 0) {
>  		fprintf(stderr, "stat: %s\n", strerror(errno));
> -		return NULL;
> +		goto out_close;
>  	}
>  	if ((st.st_mode & S_IFMT) != S_IFREG) {
>  		fprintf(stderr, "%s not a regular file\n", path);
> -		return NULL;
> +		goto out_close;
>  	}
>  
>  	if (st.st_size == 0 || st.st_size > 4096) {
>  		fprintf(stderr, "Invalid blob file size\n");
> -		return NULL;
> +		goto out_close;
>  	}
>  
>  	*size = st.st_size;
> @@ -166,15 +166,13 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  			fprintf(stderr, "Failed to read from blob
> file: %s\n",
>  					strerror(errno));
>  			free(blob);
> -			close(fd);
> -			return NULL;
> +			blob = NULL;
> +			goto out_close;
>  		}
>  		read_bytes += rc;
>  		rdptr += rc;
>  	} while (read_bytes != st.st_size);
>  
> -	close(fd);
> -
>  	if (postfix) {
>  		pl += read_bytes;
>  		*pl = ' ';
> @@ -182,6 +180,8 @@ char *ndctl_load_key_blob(const char *path, int
> *size, const char *postfix,
>  		rc = sprintf(pl, "keyhandle=%s", postfix);
>  	}
>  
> +out_close:
> +	close(fd);
>  	return blob;
>  }
>  

Re: [PATCH ndctl 2/2] libndctl.c: major and minor numbers are unsigned

[Verma, Vishal L]

On Tue, 2024-08-20 at 14:26 -0400, jmoyer@redhat.com wrote:
> From: Jeff Moyer <jmoyer@redhat.com>
> 
> Static analysis points out that the cast of bus->major and bus->minor
> to a signed type in the call to parent_dev_path could result in a
> negative number.  I sincerely doubt we'll see major and minor numbers
> that large, but let's fix it.
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>

Looks good,
Reviewed-by: Vishal Verma <vishal.l.verma@intel.com>

> ---
>  ndctl/lib/libndctl.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ndctl/lib/libndctl.c b/ndctl/lib/libndctl.c
> index ddbdd9a..f75dbd4 100644
> --- a/ndctl/lib/libndctl.c
> +++ b/ndctl/lib/libndctl.c
> @@ -710,11 +710,12 @@ NDCTL_EXPORT void ndctl_set_log_priority(struct
> ndctl_ctx *ctx, int priority)
>  	daxctl_set_log_priority(ctx->daxctl_ctx, priority);
>  }
>  
> -static char *__dev_path(char *type, int major, int minor, int
> parent)
> +static char *__dev_path(char *type, unsigned int major, unsigned int
> minor,
> +			int parent)
>  {
>  	char *path, *dev_path;
>  
> -	if (asprintf(&path, "/sys/dev/%s/%d:%d%s", type, major,
> minor,
> +	if (asprintf(&path, "/sys/dev/%s/%u:%u%s", type, major,
> minor,
>  				parent ? "/device" : "") < 0)
>  		return NULL;
>  
> @@ -723,7 +724,7 @@ static char *__dev_path(char *type, int major,
> int minor, int parent)
>  	return dev_path;
>  }
>  
> -static char *parent_dev_path(char *type, int major, int minor)
> +static char *parent_dev_path(char *type, unsigned int major,
> unsigned int minor)
>  {
>          return __dev_path(type, major, minor, 1);
>  }