[PATCH 0/5] Five small multipath-tools patches

[PATCH 0/5] Five small multipath-tools patches

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

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

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 libmultipath/prioritizers/alua.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libmultipath/prioritizers/alua.c b/libmultipath/prioritizers/alua.c
index b967ec7..39ed1c8 100644
--- a/libmultipath/prioritizers/alua.c
+++ b/libmultipath/prioritizers/alua.c
@@ -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]" : "");
-- 
1.8.4.5

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

[Sebastian Herbszt]

Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  libmultipath/prioritizers/alua.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/libmultipath/prioritizers/alua.c b/libmultipath/prioritizers/alua.c
> index b967ec7..39ed1c8 100644
> --- a/libmultipath/prioritizers/alua.c
> +++ b/libmultipath/prioritizers/alua.c
> @@ -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]" : "");

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

[PATCH 2/5] libmultipath: Simplify read_line()

[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 <bvanassche@acm.org>
---
 libmultipath/parser.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 0d4c870..b7bdfcc 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -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;
 
-- 
1.8.4.5

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

[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 <bvanassche@acm.org>
---
 libmultipath/sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libmultipath/sysfs.c b/libmultipath/sysfs.c
index e5834f9..f42cda8 100644
--- a/libmultipath/sysfs.c
+++ b/libmultipath/sysfs.c
@@ -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);
-- 
1.8.4.5

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

[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 <bvanassche@acm.org>
---
 libmultipath/parser.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index b7bdfcc..d8673bc 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -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++;
-- 
1.8.4.5

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

[Sebastian Herbszt]

Bart Van Assche wrote:
> 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 <bvanassche@acm.org>
> ---
>  libmultipath/parser.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> index b7bdfcc..d8673bc 100644
> --- a/libmultipath/parser.c
> +++ b/libmultipath/parser.c
> @@ -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++;

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

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

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 1873 bytes --]

Done,
Thanks.


On Thu, Jul 24, 2014 at 4:44 PM, Sebastian Herbszt <herbszt@gmx.de> wrote:

> Bart Van Assche wrote:
> > 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 <bvanassche@acm.org>
> > ---
> >  libmultipath/parser.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/libmultipath/parser.c b/libmultipath/parser.c
> > index b7bdfcc..d8673bc 100644
> > --- a/libmultipath/parser.c
> > +++ b/libmultipath/parser.c
> > @@ -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++;
>
> 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
>

[-- Attachment #1.2: Type: text/html, Size: 2881 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[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 <bvanassche@acm.org>
---
 libmultipath/blacklist.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 651bd7e..e5c287e 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -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;
 		}
-- 
1.8.4.5

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

[Sebastian Herbszt]

Bart Van Assche wrote:
> 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 <bvanassche@acm.org>
> ---
>  libmultipath/blacklist.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> index 651bd7e..e5c287e 100644
> --- a/libmultipath/blacklist.c
> +++ b/libmultipath/blacklist.c
> @@ -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;
>  		}

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

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

[Bryn M. Reeves]

On Thu, Jul 24, 2014 at 04:55:59PM +0200, Sebastian Herbszt wrote:
> Bart Van Assche wrote:
> > 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 ".*".
> > 
> 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.

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

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 1298 bytes --]

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

Thanks.


On Thu, Jul 24, 2014 at 6:34 PM, Bryn M. Reeves <bmr@redhat.com> wrote:

> On Thu, Jul 24, 2014 at 04:55:59PM +0200, Sebastian Herbszt wrote:
> > Bart Van Assche wrote:
> > > 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 ".*".
> > >
> > 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.
>
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2018 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 0/5] Five small multipath-tools patches

[Christophe Varoqui]

[-- Attachment #1.1: Type: text/plain, Size: 594 bytes --]

All patches seem safe and sound.
Applied.
Thanks.


On Fri, Jul 18, 2014 at 2:52 PM, Bart Van Assche <bvanassche@acm.org> wrote:

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

[-- Attachment #1.2: Type: text/html, Size: 946 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]