[PATCH 0/3] partitions/ibm: Replace strncpy() and cleanups

[PATCH 0/3] partitions/ibm: Replace strncpy() and cleanups

[Stefan Haberland]

Hi Jens,

please apply the following patches for the next merge window that remove
strncpy() from DASD partition detection. This includes some cleanups that
should increase readability of the code.

This is based on the discussion started with the patches by Justin Stitt:
https://lore.kernel.org/linux-s390/20230822-strncpy-block-partitions-cmdline-ibm-v1-1-154dea8f755c@google.com/
https://lore.kernel.org/linux-s390/20230823-strncpy-block-partitions-cmdline-ibm-v2-1-40c77f7182fc@google.com/

Thanks.

Jan Höppner (3):
  partitions/ibm: Remove unnecessary memset
  partitions/ibm: Replace strncpy() and improve readability
  partitions/ibm: Introduce defines for magic string length values

 block/partitions/ibm.c | 98 +++++++++++++++++++++++++++++-------------
 1 file changed, 68 insertions(+), 30 deletions(-)

-- 
2.39.2

[PATCH 1/3] partitions/ibm: Remove unnecessary memset

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

The data holding the volume label information is zeroed in case no valid
volume label was found. Since the label information isn't used in that
case, zeroing the data doesn't provide any value whatsoever.

Remove the unnecessary memset() call accordingly.

Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 block/partitions/ibm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c
index 403756dbd50d..49eb0e354fc4 100644
--- a/block/partitions/ibm.c
+++ b/block/partitions/ibm.c
@@ -124,8 +124,6 @@ static int find_label(struct parsed_partitions *state,
 			break;
 		}
 	}
-	if (!found)
-		memset(label, 0, sizeof(*label));
 
 	return found;
 }
-- 
2.39.2

[PATCH 2/3] partitions/ibm: Replace strncpy() and improve readability

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

strncpy() is deprecated and needs to be replaced. The volume label
information strings are not nul-terminated and strncpy() can simply be
replaced with memcpy().

To enhance the readability of find_label() alongside this change, the
following improvements are made:
- Introduce the array dasd_vollabels[] containing all information
  necessary for the label detection.
- Provide a helper function to obtain an index value corresponding to a
  volume label type. This allows the use of a switch statement to reduce
  indentation levels.
- The 'temp' variable is used to check against valid volume label types.
  In the good case, this variable already contains the volume label type
  making it unnecessary to copy the information again from e.g.
  label->vol.vollbl. Remove the 'temp' variable and the second copy as
  all information are already provided.
- Remove the 'found' variable and replace it with early returns

Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 block/partitions/ibm.c | 86 ++++++++++++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 25 deletions(-)

diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c
index 49eb0e354fc4..7b0a3f13d180 100644
--- a/block/partitions/ibm.c
+++ b/block/partitions/ibm.c
@@ -61,6 +61,43 @@ static sector_t cchhb2blk(struct vtoc_cchhb *ptr, struct hd_geometry *geo)
 		ptr->b;
 }
 
+/* Volume Label Types */
+#define DASD_VOLLBL_TYPE_VOL1 0
+#define DASD_VOLLBL_TYPE_LNX1 1
+#define DASD_VOLLBL_TYPE_CMS1 2
+
+struct dasd_vollabel {
+	char *type;
+	int idx;
+};
+
+static struct dasd_vollabel dasd_vollabels[] = {
+	[DASD_VOLLBL_TYPE_VOL1] = {
+		.type = "VOL1",
+		.idx = DASD_VOLLBL_TYPE_VOL1,
+	},
+	[DASD_VOLLBL_TYPE_LNX1] = {
+		.type = "LNX1",
+		.idx = DASD_VOLLBL_TYPE_LNX1,
+	},
+	[DASD_VOLLBL_TYPE_CMS1] = {
+		.type = "CMS1",
+		.idx = DASD_VOLLBL_TYPE_CMS1,
+	},
+};
+
+static int get_label_by_type(const char *type)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(dasd_vollabels); i++) {
+		if (!memcmp(type, dasd_vollabels[i].type, 4))
+			return dasd_vollabels[i].idx;
+	}
+
+	return -1;
+}
+
 static int find_label(struct parsed_partitions *state,
 		      dasd_information2_t *info,
 		      struct hd_geometry *geo,
@@ -70,12 +107,10 @@ static int find_label(struct parsed_partitions *state,
 		      char type[],
 		      union label_t *label)
 {
-	Sector sect;
-	unsigned char *data;
 	sector_t testsect[3];
-	unsigned char temp[5];
-	int found = 0;
 	int i, testcount;
+	Sector sect;
+	void *data;
 
 	/* There a three places where we may find a valid label:
 	 * - on an ECKD disk it's block 2
@@ -103,29 +138,27 @@ static int find_label(struct parsed_partitions *state,
 		if (data == NULL)
 			continue;
 		memcpy(label, data, sizeof(*label));
-		memcpy(temp, data, 4);
-		temp[4] = 0;
-		EBCASC(temp, 4);
+		memcpy(type, data, 4);
+		EBCASC(type, 4);
 		put_dev_sector(sect);
-		if (!strcmp(temp, "VOL1") ||
-		    !strcmp(temp, "LNX1") ||
-		    !strcmp(temp, "CMS1")) {
-			if (!strcmp(temp, "VOL1")) {
-				strncpy(type, label->vol.vollbl, 4);
-				strncpy(name, label->vol.volid, 6);
-			} else {
-				strncpy(type, label->lnx.vollbl, 4);
-				strncpy(name, label->lnx.volid, 6);
-			}
-			EBCASC(type, 4);
+		switch (get_label_by_type(type)) {
+		case DASD_VOLLBL_TYPE_VOL1:
+			memcpy(name, label->vol.volid, 6);
 			EBCASC(name, 6);
 			*labelsect = testsect[i];
-			found = 1;
+			return 1;
+		case DASD_VOLLBL_TYPE_LNX1:
+		case DASD_VOLLBL_TYPE_CMS1:
+			memcpy(name, label->lnx.volid, 6);
+			EBCASC(name, 6);
+			*labelsect = testsect[i];
+			return 1;
+		default:
 			break;
 		}
 	}
 
-	return found;
+	return 0;
 }
 
 static int find_vol1_partitions(struct parsed_partitions *state,
@@ -328,18 +361,21 @@ int ibm_partition(struct parsed_partitions *state)
 		info = NULL;
 	}
 
-	if (find_label(state, info, geo, blocksize, &labelsect, name, type,
-		       label)) {
-		if (!strncmp(type, "VOL1", 4)) {
+	if (find_label(state, info, geo, blocksize, &labelsect, name, type, label)) {
+		switch (get_label_by_type(type)) {
+		case DASD_VOLLBL_TYPE_VOL1:
 			res = find_vol1_partitions(state, geo, blocksize, name,
 						   label);
-		} else if (!strncmp(type, "LNX1", 4)) {
+			break;
+		case DASD_VOLLBL_TYPE_LNX1:
 			res = find_lnx1_partitions(state, geo, blocksize, name,
 						   label, labelsect, nr_sectors,
 						   info);
-		} else if (!strncmp(type, "CMS1", 4)) {
+			break;
+		case DASD_VOLLBL_TYPE_CMS1:
 			res = find_cms1_partitions(state, geo, blocksize, name,
 						   label, labelsect);
+			break;
 		}
 	} else if (info) {
 		/*
-- 
2.39.2

[PATCH 3/3] partitions/ibm: Introduce defines for magic string length values

[Stefan Haberland]

From: Jan Höppner <hoeppner@linux.ibm.com>

The length values for volume label type and volume label id are
hard-coded in several places. Provide defines for those values and
replace all occurrences accordingly.

Note that the length is defined and used, and not the size since the
volume label type string and volume label id string are not
nul-terminated.

Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>
Reviewed-by: Stefan Haberland <sth@linux.ibm.com>
Signed-off-by: Stefan Haberland <sth@linux.ibm.com>
---
 block/partitions/ibm.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/block/partitions/ibm.c b/block/partitions/ibm.c
index 7b0a3f13d180..82d9c4c3fb41 100644
--- a/block/partitions/ibm.c
+++ b/block/partitions/ibm.c
@@ -61,6 +61,10 @@ static sector_t cchhb2blk(struct vtoc_cchhb *ptr, struct hd_geometry *geo)
 		ptr->b;
 }
 
+/* Volume Label Type/ID Length */
+#define DASD_VOL_TYPE_LEN	4
+#define DASD_VOL_ID_LEN		6
+
 /* Volume Label Types */
 #define DASD_VOLLBL_TYPE_VOL1 0
 #define DASD_VOLLBL_TYPE_LNX1 1
@@ -91,7 +95,7 @@ static int get_label_by_type(const char *type)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(dasd_vollabels); i++) {
-		if (!memcmp(type, dasd_vollabels[i].type, 4))
+		if (!memcmp(type, dasd_vollabels[i].type, DASD_VOL_TYPE_LEN))
 			return dasd_vollabels[i].idx;
 	}
 
@@ -138,19 +142,19 @@ static int find_label(struct parsed_partitions *state,
 		if (data == NULL)
 			continue;
 		memcpy(label, data, sizeof(*label));
-		memcpy(type, data, 4);
-		EBCASC(type, 4);
+		memcpy(type, data, DASD_VOL_TYPE_LEN);
+		EBCASC(type, DASD_VOL_TYPE_LEN);
 		put_dev_sector(sect);
 		switch (get_label_by_type(type)) {
 		case DASD_VOLLBL_TYPE_VOL1:
-			memcpy(name, label->vol.volid, 6);
-			EBCASC(name, 6);
+			memcpy(name, label->vol.volid, DASD_VOL_ID_LEN);
+			EBCASC(name, DASD_VOL_ID_LEN);
 			*labelsect = testsect[i];
 			return 1;
 		case DASD_VOLLBL_TYPE_LNX1:
 		case DASD_VOLLBL_TYPE_CMS1:
-			memcpy(name, label->lnx.volid, 6);
-			EBCASC(name, 6);
+			memcpy(name, label->lnx.volid, DASD_VOL_ID_LEN);
+			EBCASC(name, DASD_VOL_ID_LEN);
 			*labelsect = testsect[i];
 			return 1;
 		default:
@@ -328,8 +332,8 @@ int ibm_partition(struct parsed_partitions *state)
 	sector_t nr_sectors;
 	dasd_information2_t *info;
 	struct hd_geometry *geo;
-	char type[5] = {0,};
-	char name[7] = {0,};
+	char type[DASD_VOL_TYPE_LEN + 1] = "";
+	char name[DASD_VOL_ID_LEN + 1] = "";
 	sector_t labelsect;
 	union label_t *label;
 
-- 
2.39.2

Re: [PATCH 0/3] partitions/ibm: Replace strncpy() and cleanups

[Stefan Haberland]

Am 15.09.23 um 15:09 schrieb Stefan Haberland:
> Hi Jens,
>
> please apply the following patches for the next merge window that remove
> strncpy() from DASD partition detection. This includes some cleanups that
> should increase readability of the code.
>
> This is based on the discussion started with the patches by Justin Stitt:
> https://lore.kernel.org/linux-s390/20230822-strncpy-block-partitions-cmdline-ibm-v1-1-154dea8f755c@google.com/
> https://lore.kernel.org/linux-s390/20230823-strncpy-block-partitions-cmdline-ibm-v2-1-40c77f7182fc@google.com/
>
> Thanks.
>
> Jan Höppner (3):
>    partitions/ibm: Remove unnecessary memset
>    partitions/ibm: Replace strncpy() and improve readability
>    partitions/ibm: Introduce defines for magic string length values
>
>   block/partitions/ibm.c | 98 +++++++++++++++++++++++++++++-------------
>   1 file changed, 68 insertions(+), 30 deletions(-)
>

Hi Jens,

polite ping.
Any objections against the patches?

regards,
Stefan

Re: [PATCH 0/3] partitions/ibm: Replace strncpy() and cleanups

[Jens Axboe]

On 10/4/23 3:59 AM, Stefan Haberland wrote:
> Am 15.09.23 um 15:09 schrieb Stefan Haberland:
>> Hi Jens,
>>
>> please apply the following patches for the next merge window that remove
>> strncpy() from DASD partition detection. This includes some cleanups that
>> should increase readability of the code.
>>
>> This is based on the discussion started with the patches by Justin Stitt:
>> https://lore.kernel.org/linux-s390/20230822-strncpy-block-partitions-cmdline-ibm-v1-1-154dea8f755c@google.com/
>> https://lore.kernel.org/linux-s390/20230823-strncpy-block-partitions-cmdline-ibm-v2-1-40c77f7182fc@google.com/
>>
>> Thanks.
>>
>> Jan H?ppner (3):
>>    partitions/ibm: Remove unnecessary memset
>>    partitions/ibm: Replace strncpy() and improve readability
>>    partitions/ibm: Introduce defines for magic string length values
>>
>>   block/partitions/ibm.c | 98 +++++++++++++++++++++++++++++-------------
>>   1 file changed, 68 insertions(+), 30 deletions(-)
>>
> 
> Hi Jens,
> 
> polite ping.
> Any objections against the patches?

Nope, just had some travel and it got lost in the inbox. Now applied.

-- 
Jens Axboe

Re: [PATCH 0/3] partitions/ibm: Replace strncpy() and cleanups

[Jens Axboe]

On Fri, 15 Sep 2023 15:09:58 +0200, Stefan Haberland wrote:
> please apply the following patches for the next merge window that remove
> strncpy() from DASD partition detection. This includes some cleanups that
> should increase readability of the code.
> 
> This is based on the discussion started with the patches by Justin Stitt:
> https://lore.kernel.org/linux-s390/20230822-strncpy-block-partitions-cmdline-ibm-v1-1-154dea8f755c@google.com/
> https://lore.kernel.org/linux-s390/20230823-strncpy-block-partitions-cmdline-ibm-v2-1-40c77f7182fc@google.com/
> 
> [...]

Applied, thanks!

[1/3] partitions/ibm: Remove unnecessary memset
      commit: d323c1a9477a82843795f10fb23f1634cea44007
[2/3] partitions/ibm: Replace strncpy() and improve readability
      commit: f5f43aae6f336ae436759144a31879375e65ed28
[3/3] partitions/ibm: Introduce defines for magic string length values
      commit: a31281acc4a4e051a0bf2f1d3556ba4deea4d2a0

Best regards,
-- 
Jens Axboe