Skip to contentSkip to conversation

Five small multipath-tools patches

PATCH In review Bart Van Assche 4 participants 13 messages Jul 18, 2014 – Jul 25, 2014 Device mapper

lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

cover Five small multipath-tools patches

This series consists of the following patches:

0001-ALUA-prioritizer-Remove-an-unused-variable.patch 0002-libmultipath-Simplify-read_line.patch 0003-libmultipath-Zero-terminate-sysfs_attr_get_value-res.patch 0004-libmultipath-Print-line-number-for-which-parsing-fai.patch 0005-libmultipath-Accept-as-a-valid-regular-expression.patch

Please note that I'm not a multipath-tools expert so a careful review of these patches would be appreciated.

author
#1 lore raw

PATCH 1/5 ALUA prioritizer: Remove an unused variable

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/prioritizers/alua.c | 2 --
 1 file changed, 2 deletions(-)
libmultipath/prioritizers/alua.c +0 -2
index b967ec7..39ed1c8 100644
@@ -55,7 +55,6 @@ get_alua_info(int fd)
 {
 	int	rc;
 	int	tpg;
-	int	aas;
 
 	rc = get_target_port_group_support(fd);
 	if (rc < 0)

@@ -72,7 +71,6 @@ get_alua_info(int fd)
 	rc = get_asymmetric_access_state(fd, tpg);
 	if (rc < 0)
 		return -ALUA_PRIO_GETAAS_FAILED;
-	aas = (rc & 0x0f);
 
 	condlog(3, "aas = %02x [%s]%s", rc, aas_print_string(rc),
 		(rc & 0x80) ? " [preferred]" : "");
Signature · 1 line
1.8.4.5
#2 lore raw

Re: [PATCH 1/5] ALUA prioritizer: Remove an unused variable

Quoted 18 lines · Bart Van Assche
Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/prioritizers/alua.c | 2 --
 1 file changed, 2 deletions(-)
libmultipath/prioritizers/alua.c +0 -1
index b967ec7..39ed1c8 100644
@@ -55,7 +55,6 @@ get_alua_info(int fd)
 {
 	int	rc;
 	int	tpg;
-	int	aas;
 
 	rc = get_target_port_group_support(fd);
 	if (rc < 0)

libmultipath/prioritizers/alua.c get_alua_info(int fd) · line 76

Quoted 7 lines
libmultipath/prioritizers/alua.c +0 -1
@@ -72,7 +71,6 @@ get_alua_info(int fd)
 	rc = get_asymmetric_access_state(fd, tpg);
 	if (rc < 0)
 		return -ALUA_PRIO_GETAAS_FAILED;
-	aas = (rc & 0x0f);
 
 	condlog(3, "aas = %02x [%s]%s", rc, aas_print_string(rc),
 		(rc & 0x80) ? " [preferred]" : "");

Question is what Hannes intended to output here with his change in a87a2aa4.

The aas field is just 4 bits. The rc variable contains more than the aas field. If I don't mistake it is d->b0 & 0x8f. So it does contain pref and aas, but not rtpg_fmt. aas_print_string on the other hand tries to check the format, but always gets rtpg_fmt as 0.

Sebastian

author
#3 lore raw

PATCH 2/5 libmultipath: Simplify read_line()

In reply to Bart Van Assche

Let read_line() zero-terminate the buffer it returns such that its callers do not have to clear that buffer before calling read_line().

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/parser.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)
libmultipath/parser.c +6 -16
index 0d4c870..b7bdfcc 100644
@@ -282,18 +282,12 @@ out:
 int
 read_line(char *buf, int size)
 {
-	int ch;
-	int count = 0;
-
-	while ((ch = fgetc(stream)) != EOF && (int) ch != '\n'
-	       && (int) ch != '\r') {
-		if (count < size)
-			buf[count] = (int) ch;
-		else
-			break;
-		count++;
-	}
-	return (ch == EOF) ? 0 : 1;
+	char *p;
+
+	if (fgets(buf, size, stream) == NULL)
+		return 0;
+	strtok_r(buf, "\n\r", &p);
+	return 1;
 }
 
 vector

@@ -341,7 +335,6 @@ read_value_block(void)
 			}
 			free_strvec(vec);
 		}
-		memset(buf, 0, MAXBUF);
 	}
 	FREE(buf);
 	return elements;

@@ -379,7 +372,6 @@ alloc_value_block(vector strvec, void (*alloc_func) (vector))
 
 			free_strvec(vec);
 		}
-		memset(buf, 0, MAXBUF);
 	}
 	FREE(buf);
 	return 0;

@@ -489,8 +481,6 @@ process_stream(vector keywords)
 	while (read_line(buf, MAXBUF)) {
 		line_nr++;
 		strvec = alloc_strvec(buf);
-		memset(buf,0, MAXBUF);
-
 		if (!strvec)
 			continue;
Signature · 1 line
1.8.4.5
author
#4 lore raw

PATCH 3/5 libmultipath: Zero-terminate sysfs_attr_get_value() result

In reply to Bart Van Assche

The callers of sysfs_attr_get_value() expect a '\0'-terminated string. Hence terminate the string returned by this function with '\0'. Detected by Valgrind.

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/sysfs.c | 2 ++
 1 file changed, 2 insertions(+)
libmultipath/sysfs.c +2 -0
index e5834f9..f42cda8 100644
@@ -88,6 +88,8 @@ ssize_t sysfs_attr_get_value(struct udev_device *dev, const char *attr_name,
 	} else if (size == value_len) {
 		condlog(4, "overflow while reading from %s", devpath);
 		size = 0;
+	} else {
+		value[size] = '\0';
 	}
 
 	close(fd);
Signature · 1 line
1.8.4.5
author
#5 lore raw

PATCH 4/5 libmultipath: Print line number for which parsing failed

In reply to Bart Van Assche

Make it easier to figure out which line contains a syntax error by printing the line number and the offending line itself.

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/parser.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
libmultipath/parser.c +8 -3
index b7bdfcc..d8673bc 100644
@@ -460,7 +460,7 @@ int
 process_stream(vector keywords)
 {
 	int i;
-	int r = 0;
+	int r = 0, t;
 	struct keyword *keyword;
 	char *str;
 	char *buf;

@@ -501,8 +501,13 @@ process_stream(vector keywords)
 						free_strvec(strvec);
 						goto out;
 				}
-				if (keyword->handler)
-					r += (*keyword->handler) (strvec);
+				if (keyword->handler) {
+					t = (*keyword->handler) (strvec);
+					r += t;
+					if (t)
+						condlog(1, "multipath.conf +%d parsing failed: %s",
+							line_nr, buf);
+				}
 
 				if (keyword->sub) {
 					kw_level++;
Signature · 1 line
1.8.4.5
#6 lore raw

Re: [PATCH 4/5] libmultipath: Print line number for which parsing failed

Quoted 27 lines · Bart Van Assche

Make it easier to figure out which line contains a syntax error by printing the line number and the offending line itself.

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/parser.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
libmultipath/parser.c +1 -3
index b7bdfcc..d8673bc 100644
@@ -460,7 +460,7 @@ int
 process_stream(vector keywords)
 {
 	int i;
-	int r = 0;
+	int r = 0, t;
 	struct keyword *keyword;
 	char *str;
 	char *buf;

@@ -501,8 +501,13 @@ process_stream(vector keywords)
 						free_strvec(strvec);
 						goto out;
 				}
-				if (keyword->handler)
-					r += (*keyword->handler) (strvec);

libmultipath/parser.c process_stream(vector keywords) · line 513

Quoted 10 lines
libmultipath/parser.c +7 -0
+				if (keyword->handler) {
+					t = (*keyword->handler) (strvec);
+					r += t;
+					if (t)
+						condlog(1, "multipath.conf +%d parsing failed: %s",
+							line_nr, buf);
+				}
 
 				if (keyword->sub) {
 					kw_level++;

Further below
condlog(1, "multipath.conf +%d, invalid keyword: %s", line_nr, str); is used, so maybe also add a comma after the line number too.

Sebastian

#7 lore raw

Re: [PATCH 4/5] libmultipath: Print line number for which parsing failed

Done,
Thanks.

Quoted 46 lines · Sebastian Herbszt
Quoted 22 lines · Bart Van Assche

Make it easier to figure out which line contains a syntax error by printing the line number and the offending line itself.

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/parser.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)
libmultipath/parser.c +1 -1
index b7bdfcc..d8673bc 100644
@@ -460,7 +460,7 @@ int
 process_stream(vector keywords)
 {
      int i;
-     int r = 0;
+     int r = 0, t;
      struct keyword *keyword;
      char *str;
      char *buf;

libmultipath/parser.c process_stream(vector keywords) · line 508

Quoted 10 lines
libmultipath/parser.c +5 -2
@@ -501,8 +501,13 @@ process_stream(vector keywords)
                                              free_strvec(strvec);
                                              goto out;
                              }
-                             if (keyword->handler)
-                                     r += (*keyword->handler) (strvec);
+                             if (keyword->handler) {
+                                     t = (*keyword->handler) (strvec);
+                                     r += t;
+                                     if (t)
+                                             condlog(1, "multipath.conf

+%d parsing failed: %s",

libmultipath/parser.c process_stream(vector keywords) · line 513

Quoted 5 lines
libmultipath/parser.c +2 -0
+                                                     line_nr, buf);
+                             }
 
                              if (keyword->sub) {
                                      kw_level++;

Further below
condlog(1, "multipath.conf +%d, invalid keyword: %s", line_nr, str); is used, so maybe also add a comma after the line number too.

Sebastian

author
#8 lore raw

PATCH 5/5 libmultipath: Accept "*" as a valid regular expression

In reply to Bart Van Assche

Inside libmultipath regcomp() is used to compile regular expressions specified in /etc/multipath.conf. Many multipath.conf examples contain 'product_type "*"'. However, "*" is not a valid POSIX regular expression. Hence this patch that changes the regular expression "*" into ".*".

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/blacklist.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
libmultipath/blacklist.c +11 -4
index 651bd7e..e5c287e 100644
@@ -63,6 +63,13 @@ alloc_ble_device (vector blist)
 	return 0;
 }
 
+static int lm_regcomp(regex_t *preg, const char *pattern, int cflags)
+{
+	if (strcmp(pattern, "*") == 0)
+		pattern = ".*";
+	return regcomp(preg, pattern, cflags);
+}
+
 extern int
 set_ble_device (vector blist, char * vendor, char * product, int origin)
 {

@@ -77,16 +84,16 @@ set_ble_device (vector blist, char * vendor, char * product, int origin)
 		return 1;
 
 	if (vendor) {
-		if (regcomp(&ble->vendor_reg, vendor,
-			    REG_EXTENDED|REG_NOSUB)) {
+		if (lm_regcomp(&ble->vendor_reg, vendor,
+			       REG_EXTENDED|REG_NOSUB)) {
 			FREE(vendor);
 			return 1;
 		}
 		ble->vendor = vendor;
 	}
 	if (product) {
-		if (regcomp(&ble->product_reg, product,
-			    REG_EXTENDED|REG_NOSUB)) {
+		if (lm_regcomp(&ble->product_reg, product,
+			       REG_EXTENDED|REG_NOSUB)) {
 			FREE(product);
 			return 1;
 		}
Signature · 1 line
1.8.4.5
#9 lore raw

Re: [PATCH 5/5] libmultipath: Accept "*" as a valid regular expression

Quoted 39 lines · Bart Van Assche

Inside libmultipath regcomp() is used to compile regular expressions specified in /etc/multipath.conf. Many multipath.conf examples contain 'product_type "*"'. However, "*" is not a valid POSIX regular expression. Hence this patch that changes the regular expression "*" into ".*".

Signed-off-by Bart Van Assche <[email protected]>
---
 libmultipath/blacklist.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)
libmultipath/blacklist.c +9 -2
index 651bd7e..e5c287e 100644
@@ -63,6 +63,13 @@ alloc_ble_device (vector blist)
 	return 0;
 }
 
+static int lm_regcomp(regex_t *preg, const char *pattern, int cflags)
+{
+	if (strcmp(pattern, "*") == 0)
+		pattern = ".*";
+	return regcomp(preg, pattern, cflags);
+}
+
 extern int
 set_ble_device (vector blist, char * vendor, char * product, int origin)
 {

@@ -77,16 +84,16 @@ set_ble_device (vector blist, char * vendor, char * product, int origin)
 		return 1;
 
 	if (vendor) {
-		if (regcomp(&ble->vendor_reg, vendor,
-			    REG_EXTENDED|REG_NOSUB)) {
+		if (lm_regcomp(&ble->vendor_reg, vendor,
+			       REG_EXTENDED|REG_NOSUB)) {
 			FREE(vendor);
 			return 1;
 		}

libmultipath/blacklist.c set_ble_device (vector blist, char * vendor, char * product, int origin) · line 99

Quoted 10 lines
libmultipath/blacklist.c +2 -2
 		ble->vendor = vendor;
 	}
 	if (product) {
-		if (regcomp(&ble->product_reg, product,
-			    REG_EXTENDED|REG_NOSUB)) {
+		if (lm_regcomp(&ble->product_reg, product,
+			       REG_EXTENDED|REG_NOSUB)) {
 			FREE(product);
 			return 1;
 		}

Is this change really required? With patch 4 we now get a proper error:
multipath.conf +14 parsing failed: vendor "*"
multipath.conf +15 parsing failed: product "*"
error parsing config file

I think it should be enough to modify the man page to mention vendor/product are both regular expressions. This change might also confuse users since this automagic "*" to ".*" only applies to the blacklist exceptions.

Sebastian

#10 lore raw

Re: [PATCH 5/5] libmultipath: Accept "*" as a valid regular expression

Quoted 6 lines · Sebastian Herbszt
Quoted 5 lines · Bart Van Assche

Inside libmultipath regcomp() is used to compile regular expressions specified in /etc/multipath.conf. Many multipath.conf examples contain 'product_type "*"'. However, "*" is not a valid POSIX regular expression. Hence this patch that changes the regular expression "*" into ".*".

Quoted 8 lines

Is this change really required? With patch 4 we now get a proper error:
multipath.conf +14 parsing failed: vendor "*"
multipath.conf +15 parsing failed: product "*"
error parsing config file

I think it should be enough to modify the man page to mention vendor/product are both regular expressions. This change might also confuse users since this automagic "*" to ".*" only applies to the blacklist exceptions.

Agreed; the examples are broken here so let's fix them rather than add magic parsing that will only come back to confuse people in the future.

Regards,
Bryn.

#11 lore raw

Re: [PATCH 5/5] libmultipath: Accept "*" as a valid regular expression

Right, I agree with your comments.
The patch is reverted.

Thanks.

Quoted 31 lines · Bryn M. Reeves
Quoted 4 lines · Sebastian Herbszt
Quoted 3 lines · Bart Van Assche

Inside libmultipath regcomp() is used to compile regular expressions specified in /etc/multipath.conf. Many multipath.conf examples contain 'product_type "*"'. However, "*" is not a valid POSIX regular

expression.

Quoted 8 lines
Quoted 2 lines

Hence this patch that changes the regular expression "*" into ".*".

Is this change really required? With patch 4 we now get a proper error:
multipath.conf +14 parsing failed: vendor "*"
multipath.conf +15 parsing failed: product "*"
error parsing config file

I think it should be enough to modify the man page to mention

vendor/product

Quoted 1 line

are both regular expressions. This change might also confuse users since

this

Quoted 1 line

automagic "*" to ".*" only applies to the blacklist exceptions.

Agreed; the examples are broken here so let's fix them rather than add magic
parsing that will only come back to confuse people in the future.

Regards,
Bryn.

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel

#12 lore raw
In reply to Bart Van Assche

All patches seem safe and sound.
Applied.
Thanks.

Quoted 11 lines · Bart Van Assche

This series consists of the following patches:

0001-ALUA-prioritizer-Remove-an-unused-variable.patch 0002-libmultipath-Simplify-read_line.patch 0003-libmultipath-Zero-terminate-sysfs_attr_get_value-res.patch 0004-libmultipath-Print-line-number-for-which-parsing-fai.patch 0005-libmultipath-Accept-as-a-valid-regular-expression.patch

Please note that I'm not a multipath-tools expert so a careful review of these patches would be appreciated.