[PATCH v2 0/5] Fixes for config file parsing

[PATCH v2 0/5] Fixes for config file parsing

[Martin Wilck]

This series was motivated by the real-world problem that a user couldn't
figure out how to write a blacklist entry for a device called '1.8" SSD'.
Fixing this for good turned out to be a little tricky, therefore I also
added a test suite.

Changes since v1:
 - fixed a problem with parsing the command strings from cli handlers.

Martin Wilck (5):
  tests: add unit tests for config file parser
  libmultipath: config parser: don't strip whitepace between quotes
  libmultipath: config parser: Allow '"' in strings
  libmultipath: config parser: fix corner case for double quotes
  multipath.conf(5): improve syntax documentation

 libmultipath/parser.c      |  60 ++++--
 libmultipath/parser.h      |   1 +
 multipath/multipath.conf.5 |  17 ++
 multipathd/cli.c           |   2 +-
 tests/Makefile             |   2 +-
 tests/globals.c            |   1 +
 tests/parser.c             | 474 +++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 540 insertions(+), 17 deletions(-)
 create mode 100644 tests/parser.c

-- 
2.16.1

[PATCH v2 1/5] tests: add unit tests for config file parser

[Martin Wilck]

Add test cases for parsing the config file.

Some of these tests currently fail. The patches that follow fix them.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 tests/Makefile  |   2 +-
 tests/globals.c |   1 +
 tests/parser.c  | 479 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 481 insertions(+), 1 deletion(-)
 create mode 100644 tests/parser.c

diff --git a/tests/Makefile b/tests/Makefile
index 7ae6b9012b5a..81f5518b05d0 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -3,7 +3,7 @@ include ../Makefile.inc
 CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
 LIBDEPS += -L$(multipathdir) -lmultipath -lcmocka
 
-TESTS := uevent
+TESTS := uevent parser
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
diff --git a/tests/globals.c b/tests/globals.c
index 96a56515fd09..80f57bd3639a 100644
--- a/tests/globals.c
+++ b/tests/globals.c
@@ -6,6 +6,7 @@ struct udev *udev;
 int logsink = 0;
 struct config conf = {
 	.uid_attrs = "sd:ID_BOGUS",
+	.verbosity = 4,
 };
 
 struct config *get_multipath_config(void)
diff --git a/tests/parser.c b/tests/parser.c
new file mode 100644
index 000000000000..8e73cebd878a
--- /dev/null
+++ b/tests/parser.c
@@ -0,0 +1,479 @@
+/*
+ * Copyright (c) 2018 SUSE Linux GmbH
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ *
+ */
+
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <cmocka.h>
+// #include "list.h"
+#include "parser.h"
+#include "vector.h"
+
+#include "globals.c"
+
+/* Set these to 1 to get success for current broken behavior */
+/* Strip leading whitespace between quotes */
+#define LSTRIP_QUOTED_WSP 0
+/* Stop parsing at 2nd quote */
+#define TWO_QUOTES_ONLY 0
+
+static bool is_quote(const char *s)
+{
+	return *s == '"';
+}
+
+static char *test_file = "test.conf";
+
+/* Missing declaration */
+int validate_config_strvec(vector strvec, char *file);
+
+/* Stringify helpers */
+#define _str_(x) #x
+#define str(x) _str_(x)
+
+static int setup(void **state)
+{
+	return 0;
+}
+
+static int teardown(void **state)
+{
+	return 0;
+}
+
+static void test01(void **state)
+{
+	vector v = alloc_strvec("keyword value");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 2);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test02(void **state)
+{
+	vector v = alloc_strvec("keyword \"value\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "value");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test03(void **state)
+{
+	vector v = alloc_strvec("keyword value\n");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 2);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test04(void **state)
+{
+	vector v = alloc_strvec("keyword \t   value   \t \n   ");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 2);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test05(void **state)
+{
+	vector v = alloc_strvec("keyword \t   value   \t ! comment  ");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 2);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test06(void **state)
+{
+	vector v = alloc_strvec("keyword \t   value   # \n comment  ");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 2);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test07(void **state)
+{
+	vector v = alloc_strvec("keyword \t   value   more  ");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 3);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+	assert_string_equal(VECTOR_SLOT(v, 2), "more");
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test08(void **state)
+{
+#define QUOTED08 "  value   more  "
+#define QUOTED08B "value   more  "
+	vector v = alloc_strvec("keyword \t \"" QUOTED08 "\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+#if LSTRIP_QUOTED_WSP
+	assert_string_equal(VECTOR_SLOT(v, 2), QUOTED08B);
+#else
+	assert_string_equal(VECTOR_SLOT(v, 2), QUOTED08);
+#endif
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+#if LSTRIP_QUOTED_WSP
+	assert_string_equal(val, QUOTED08B);
+#else
+	assert_string_equal(val, QUOTED08);
+#endif
+	free(val);
+	free_strvec(v);
+}
+
+static void test09(void **state)
+{
+#define QUOTED09 "value # more"
+	vector v = alloc_strvec("keyword \"" QUOTED09 "\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), QUOTED09);
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, QUOTED09);
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test10(void **state)
+{
+#define QUOTED10 "value ! more"
+	vector v = alloc_strvec("keyword \"" QUOTED10 "\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), QUOTED10);
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, QUOTED10);
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test11(void **state)
+{
+#define QUOTED11 "value comment"
+	vector v = alloc_strvec("keyword\"" QUOTED11 "\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), QUOTED11);
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, QUOTED11);
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test12(void **state)
+{
+	vector v = alloc_strvec("key\"word\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "key");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "word");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "word");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test13(void **state)
+{
+	vector v = alloc_strvec("keyword value \"quoted\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 5);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_string_equal(VECTOR_SLOT(v, 1), "value");
+	assert_true(is_quote(VECTOR_SLOT(v, 2)));;
+	assert_string_equal(VECTOR_SLOT(v, 3), "quoted");
+	assert_true(is_quote(VECTOR_SLOT(v, 4)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "value");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test14(void **state)
+{
+	vector v = alloc_strvec("keyword \"value \"  comment\"\"");
+	char *val;
+
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 7);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "value ");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+	assert_string_equal(VECTOR_SLOT(v, 4), "comment");
+	assert_true(is_quote(VECTOR_SLOT(v, 5)));;
+	assert_true(is_quote(VECTOR_SLOT(v, 6)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "value ");
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test15(void **state)
+{
+#define QUOTED15 "word  value\n  comment"
+	vector v = alloc_strvec("key\"" QUOTED15 "\"");
+	char *val;
+
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "key");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), QUOTED15);
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+
+	val = set_value(v);
+	assert_string_equal(val, QUOTED15);
+
+	free(val);
+	free_strvec(v);
+}
+
+static void test16(void **state)
+{
+	vector v = alloc_strvec("keyword \"2.5\"\" SSD\"");
+	char *val;
+
+#if TWO_QUOTES_ONLY
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 6);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "2.5");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+	assert_string_equal(VECTOR_SLOT(v, 4), "SSD");
+	assert_true(is_quote(VECTOR_SLOT(v, 5)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "2.5");
+#else
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "2.5\" SSD");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "2.5\" SSD");
+#endif
+	free(val);
+	free_strvec(v);
+}
+
+static void test17(void **state)
+{
+	vector v = alloc_strvec("keyword \"\"\"\"\" is empty\"");
+ 	char *val;
+#if TWO_QUOTES_ONLY
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 6);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_true(is_quote(VECTOR_SLOT(v, 2)));;
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+#if LSTRIP_QUOTED_WSP
+	assert_string_equal(VECTOR_SLOT(v, 4), "is empty");
+#else
+	assert_string_equal(VECTOR_SLOT(v, 4), " is empty");
+#endif
+	assert_true(is_quote(VECTOR_SLOT(v, 5)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "");
+#else
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "\"\" is empty");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "\"\" is empty");
+#endif
+	free(val);
+	free_strvec(v);
+}
+
+static void test18(void **state)
+{
+	vector v = alloc_strvec("keyword \"\"\"\"");
+ 	char *val;
+#if TWO_QUOTES_ONLY
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 5);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_true(is_quote(VECTOR_SLOT(v, 2)));;
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+	assert_true(is_quote(VECTOR_SLOT(v, 4)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "");
+#else
+	assert_int_equal(validate_config_strvec(v, test_file), 0);
+	assert_int_equal(VECTOR_SIZE(v), 4);
+	assert_string_equal(VECTOR_SLOT(v, 0), "keyword");
+	assert_true(is_quote(VECTOR_SLOT(v, 1)));;
+	assert_string_equal(VECTOR_SLOT(v, 2), "\"");
+	assert_true(is_quote(VECTOR_SLOT(v, 3)));;
+
+	val = set_value(v);
+	assert_string_equal(val, "\"");
+#endif
+	free(val);
+	free_strvec(v);
+}
+
+int test_config_parser(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test01),
+		cmocka_unit_test(test02),
+		cmocka_unit_test(test03),
+		cmocka_unit_test(test04),
+		cmocka_unit_test(test05),
+		cmocka_unit_test(test06),
+		cmocka_unit_test(test07),
+		cmocka_unit_test(test08),
+		cmocka_unit_test(test09),
+		cmocka_unit_test(test10),
+		cmocka_unit_test(test11),
+		cmocka_unit_test(test12),
+		cmocka_unit_test(test13),
+		cmocka_unit_test(test14),
+		cmocka_unit_test(test15),
+		cmocka_unit_test(test16),
+		cmocka_unit_test(test17),
+		cmocka_unit_test(test18),
+	};
+	return cmocka_run_group_tests(tests, setup, teardown);
+}
+
+int main(void)
+{
+	int ret = 0;
+
+	ret += test_config_parser();
+	return ret;
+}
-- 
2.16.1

[PATCH v2 2/5] libmultipath: config parser: don't strip whitepace between quotes

[Martin Wilck]

Between double quotes, the parser currently strips leading (but not
trailing) whitespace. That's inconsistent and unexpected. Fix it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 5caa2019a1a4..3d9656f47945 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -262,7 +262,8 @@ alloc_strvec(char *string)
 		}
 		vector_set_slot(strvec, token);
 
-		while ((isspace((int) *cp) || !isascii((int) *cp))
+		while ((!in_string &&
+			(isspace((int) *cp) || !isascii((int) *cp)))
 		       && *cp != '\0')
 			cp++;
 		if (*cp == '\0' || *cp == '!' || *cp == '#')
-- 
2.16.1

[PATCH v2 3/5] libmultipath: config parser: Allow '"' in strings

[Martin Wilck]

We have seen model strings lile '2.5" SSD' which can't be parsed
by the current config parser. This patch fixes this by allowing
'""' to represent a double quote character inside a a string.
The above model string could now be entered in the config file like this:

blacklist {
	  vendor SomeCorp
	  product "2.5"" SSD"
}

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 3d9656f47945..21151a16ad74 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -219,11 +219,13 @@ alloc_strvec(char *string)
 
 	in_string = 0;
 	while (1) {
+		int two_quotes = 0;
+
 		if (!vector_alloc_slot(strvec))
 			goto out;
 
 		start = cp;
-		if (*cp == '"') {
+		if (*cp == '"' && !(in_string && *(cp + 1) == '"')) {
 			cp++;
 			token = MALLOC(2);
 
@@ -246,11 +248,23 @@ alloc_strvec(char *string)
 			*(token + 1) = '\0';
 			cp++;
 		} else {
+
+		move_on:
 			while ((in_string ||
 				(!isspace((int) *cp) && isascii((int) *cp) &&
 				 *cp != '!' && *cp != '#' && *cp != '{' &&
 				 *cp != '}')) && *cp != '\0' && *cp != '"')
 				cp++;
+
+			/* Two consecutive double quotes - don't end string */
+			if (in_string && *cp == '"') {
+				if (*(cp + 1) == '"') {
+					two_quotes = 1;
+					cp += 2;
+					goto move_on;
+				}
+			}
+
 			strlen = cp - start;
 			token = MALLOC(strlen + 1);
 
@@ -259,6 +273,16 @@ alloc_strvec(char *string)
 
 			memcpy(token, start, strlen);
 			*(token + strlen) = '\0';
+
+			/* Replace "" by " */
+			if (two_quotes) {
+				char *qq = strstr(token, "\"\"");
+				while (qq != NULL) {
+					memmove(qq + 1, qq + 2,
+						strlen + 1 - (qq + 2 - token));
+					qq = strstr(qq + 1, "\"\"");
+				}
+			}
 		}
 		vector_set_slot(strvec, token);
 
-- 
2.16.1

[PATCH v2 4/5] libmultipath: config parser: fix corner case for double quotes

[Martin Wilck]

A corner case of the previous patch are strings starting with a double quote,
such as '"prepended to itself is false" prepended to itself is false' or
'"" is the empty string', and in particular, the string '"' ("\"" in C
notation), which is indistinguishable from the "QUOTE" token in the parsed strvec.

This patch fixes that by introducing a special token that can't occur as part
of a normal string to indicate the beginning and end of a quoted string.

'"' is admittedly not a very likely keyword value for multipath.conf, but
a) this is a matter of correctness, b) we didn't think of '2.5"' before, either, and
c) the (*str != '"') expressions would need to be patched anyway to fix the
'string starting with "' case.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/parser.c | 31 ++++++++++++++++++-------------
 libmultipath/parser.h |  1 +
 multipathd/cli.c      |  2 +-
 tests/parser.c        |  5 -----
 4 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/libmultipath/parser.c b/libmultipath/parser.c
index 21151a16ad74..cee1c9681361 100644
--- a/libmultipath/parser.c
+++ b/libmultipath/parser.c
@@ -186,6 +186,12 @@ snprint_keyword(char *buff, int len, char *fmt, struct keyword *kw,
 	return fwd;
 }
 
+static const char quote_marker[] = { '\0', '"', '\0' };
+bool is_quote(const char* token)
+{
+	return !memcmp(token, quote_marker, sizeof(quote_marker));
+}
+
 vector
 alloc_strvec(char *string)
 {
@@ -227,13 +233,12 @@ alloc_strvec(char *string)
 		start = cp;
 		if (*cp == '"' && !(in_string && *(cp + 1) == '"')) {
 			cp++;
-			token = MALLOC(2);
+			token = MALLOC(sizeof(quote_marker));
 
 			if (!token)
 				goto out;
 
-			*(token) = '"';
-			*(token + 1) = '\0';
+			memcpy(token, quote_marker, sizeof(quote_marker));
 			if (in_string)
 				in_string = 0;
 			else
@@ -324,13 +329,13 @@ set_value(vector strvec)
 			(char *)VECTOR_SLOT(strvec, 0));
 		return NULL;
 	}
-	size = strlen(str);
-	if (size == 0) {
-		condlog(0, "option '%s' has empty value",
-			(char *)VECTOR_SLOT(strvec, 0));
-		return NULL;
-	}
-	if (*str != '"') {
+	if (!is_quote(str)) {
+		size = strlen(str);
+		if (size == 0) {
+			condlog(0, "option '%s' has empty value",
+				(char *)VECTOR_SLOT(strvec, 0));
+			return NULL;
+		}
 		alloc = MALLOC(sizeof (char) * (size + 1));
 		if (alloc)
 			memcpy(alloc, str, size);
@@ -354,7 +359,7 @@ set_value(vector strvec)
 				(char *)VECTOR_SLOT(strvec, 0));
 			return NULL;
 		}
-		if (*str == '"')
+		if (is_quote(str))
 			break;
 		tmp = alloc;
 		/* The first +1 is for the NULL byte. The rest are for the
@@ -460,7 +465,7 @@ validate_config_strvec(vector strvec, char *file)
 			(char *)VECTOR_SLOT(strvec, 0), line_nr, file);
 		return -1;
 	}
-	if (*str != '"') {
+	if (!is_quote(str)) {
 		if (VECTOR_SIZE(strvec) > 2)
 			condlog(0, "ignoring extra data starting with '%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, 2), line_nr, file);
 		return 0;
@@ -472,7 +477,7 @@ validate_config_strvec(vector strvec, char *file)
 				line_nr, file);
 			return -1;
 		}
-		if (*str == '"') {
+		if (is_quote(str)) {
 			if (VECTOR_SIZE(strvec) > i + 1)
 				condlog(0, "ignoring extra data starting with '%s' on line %d of %s", (char *)VECTOR_SLOT(strvec, (i + 1)), line_nr, file);
 			return 0;
diff --git a/libmultipath/parser.h b/libmultipath/parser.h
index 0a747507d7be..62906e98c1f7 100644
--- a/libmultipath/parser.h
+++ b/libmultipath/parser.h
@@ -81,5 +81,6 @@ extern int process_file(struct config *conf, char *conf_file);
 extern struct keyword * find_keyword(vector keywords, vector v, char * name);
 int snprint_keyword(char *buff, int len, char *fmt, struct keyword *kw,
 		    const void *data);
+bool is_quote(const char* token);
 
 #endif
diff --git a/multipathd/cli.c b/multipathd/cli.c
index f10f862cd14f..bf25b41fd5dd 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -279,7 +279,7 @@ get_cmdvec (char * cmd, vector *v)
 	}
 
 	vector_foreach_slot(strvec, buff, i) {
-		if (*buff == '"')
+		if (is_quote(buff))
 			continue;
 		if (get_param) {
 			get_param = 0;
diff --git a/tests/parser.c b/tests/parser.c
index 8e73cebd878a..a7e759838495 100644
--- a/tests/parser.c
+++ b/tests/parser.c
@@ -34,11 +34,6 @@
 /* Stop parsing at 2nd quote */
 #define TWO_QUOTES_ONLY 0
 
-static bool is_quote(const char *s)
-{
-	return *s == '"';
-}
-
 static char *test_file = "test.conf";
 
 /* Missing declaration */
-- 
2.16.1

[PATCH v2 5/5] multipath.conf(5): improve syntax documentation

[Martin Wilck]

Describe the syntax of attribute / value pairs, comments, and quoted
strings, as well as the peculiarities of section beginnings and ends.
Also describe the newly added '""' feature.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.conf.5 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index ab151e720d75..c4d0789475a3 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -67,6 +67,23 @@ recognized keywords for attributes or subsections depend on the
 section in which they occur.
 .LP
 .
+\fB<attribute>\fR and \fB<value>\fR must be on a single line.
+\fB<attribute>\fR is one of the keywords listed in this man page.
+\fB<value>\fR is either a simple word (containing no whitespace and none of the
+characters '\(dq', '#', and '!') or \fIone\fR string enclosed in double
+quotes ("..."). Outside a quoted string, text starting with '#', and '!' is
+regarded as a comment and ignored until the end of the line. Inside a quoted
+string, '#' and '!' are normal characters, and whitespace is preserved.
+To represent a double quote character inside a double quoted string, use two
+consecutive double quotes ('""'). Thus '2.5\(dq SSD' can be written as "2.5"" SSD".
+.LP
+.
+Opening braces ('{') must follow the (sub)section name on the same line. Closing
+braces ('}') that mark the end of a (sub)section must be the only non-whitespace
+character on the line. Whitespace is ignored except inside double quotes, thus
+the indentation shown in the above example is helpful for human readers but
+not mandatory.
+.LP
 .
 The following \fIsection\fP keywords are recognized:
 .TP 17
-- 
2.16.1

Re: [PATCH v2 0/5] Fixes for config file parsing

[Benjamin Marzinski]

On Thu, Mar 08, 2018 at 12:26:15AM +0100, Martin Wilck wrote:
> This series was motivated by the real-world problem that a user couldn't
> figure out how to write a blacklist entry for a device called '1.8" SSD'.
> Fixing this for good turned out to be a little tricky, therefore I also
> added a test suite.
> 
> Changes since v1:
>  - fixed a problem with parsing the command strings from cli handlers.
> 

Reviewed-by: Benjmain Marzinski <bmarzins@redhat.com>

for the set.

> Martin Wilck (5):
>   tests: add unit tests for config file parser
>   libmultipath: config parser: don't strip whitepace between quotes
>   libmultipath: config parser: Allow '"' in strings
>   libmultipath: config parser: fix corner case for double quotes
>   multipath.conf(5): improve syntax documentation
> 
>  libmultipath/parser.c      |  60 ++++--
>  libmultipath/parser.h      |   1 +
>  multipath/multipath.conf.5 |  17 ++
>  multipathd/cli.c           |   2 +-
>  tests/Makefile             |   2 +-
>  tests/globals.c            |   1 +
>  tests/parser.c             | 474 +++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 540 insertions(+), 17 deletions(-)
>  create mode 100644 tests/parser.c
> 
> -- 
> 2.16.1

Re: [PATCH v2 3/5] libmultipath: config parser: Allow '"' in strings

[Xose Vazquez Perez]

On 03/08/2018 12:26 AM, Martin Wilck wrote:

> We have seen model strings lile '2.5" SSD' which can't be parsed
> by the current config parser. This patch fixes this by allowing
> '""' to represent a double quote character inside a a string.
> The above model string could now be entered in the config file like this:
> 
> blacklist {
> 	  vendor SomeCorp
> 	  product "2.5"" SSD"
> }


Could this work in the first place?

          product "2.5\" SSD"

Re: [PATCH v2 3/5] libmultipath: config parser: Allow '"' in strings

[Martin Wilck]

On Fr, 2018-03-23 at 17:59 +0100, Xose Vazquez Perez wrote:
> On 03/08/2018 12:26 AM, Martin Wilck wrote:
> 
> > We have seen model strings lile '2.5" SSD' which can't be parsed
> > by the current config parser. This patch fixes this by allowing
> > '""' to represent a double quote character inside a a string.
> > The above model string could now be entered in the config file like
> > this:
> > 
> > blacklist {
> > 	  vendor SomeCorp
> > 	  product "2.5"" SSD"
> > }
> 
> 
> Could this work in the first place?
> 
>           product "2.5\" SSD"

I'm not sure what you mean. For the current code, the answer is "no".
The parser doesn't treat '\' special in any way. For the parser, the
result of the above is the tuple ['product', '2.5\', 'SSD']. The config
file parser would discard the last tuple element (the cli command
parser takes more than 2 tuple elements, but not the config file
parser). The regexp parser would bail out on the lone backslash in
'2.5\'.

If you meant to say that, instead of using '""', I could have changed
the parser to use '\"' to represent a single double-quote character,
yes, that would have been possible, at the cost of a much bigger and
more error-prone rewrite of the parsing code.

Moreover, I think using '\"' has an addtional disadvantage compared to
'""'. As you know, the values for many options are regular expressions,
and the backslash has special meaning in regexps. Figuring out the
correct number of backslashes for complex regexps is hard anyway, and
if we give '\' special meaning in the option parser, too, we'd treat it
specially on two separate layers, making it even harder for users. The
double-quote character, OTOH, has no special meaning in regexps, so
that the two quoting mechanisms on the two layers are orthogonal.

Regards
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel