[Cluster-devel] [PATCH] mkfs.gfs2: Always validate the locktable option

[Cluster-devel] [PATCH] mkfs.gfs2: Always validate the locktable option

[Andrew Price]

When mkfs.gfs2 is run with -p lock_nolock and the -t <locktable> option
is also used, validation of the locktable is skipped. Make sure the -t
option is always checked whether it's required (with lock_dlm/lock_gulm)
or not (with lock_nolock).

Also add some tests which ensure the locktable is validated in this
scenario.

Signed-off-by: Andrew Price <anprice@redhat.com>
---
 gfs2/mkfs/main_mkfs.c | 66 ++++++++++++++++++++++++++++-----------------------
 tests/mkfs.at         |  6 ++++-
 2 files changed, 41 insertions(+), 31 deletions(-)

diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
index 48345b5..c1f7217 100644
--- a/gfs2/mkfs/main_mkfs.c
+++ b/gfs2/mkfs/main_mkfs.c
@@ -373,45 +373,51 @@ static void opts_get(int argc, char *argv[], struct mkfs_opts *opts)
  *
  */
 
-static void test_locking(const char *lockproto, const char *locktable)
+static void test_locking(struct mkfs_opts *opts)
 {
 	const char *c;
 	/* Translators: A lock table is a string identifying a gfs2 file system
 	 * in a cluster, e.g. cluster_name:fs_name */
 	const char *errprefix = _("Invalid lock table:");
+	int table_required = (strcmp(opts->lockproto, "lock_gulm") == 0)
+	                  || (strcmp(opts->lockproto, "lock_dlm") == 0);
 
-	if (strcmp(lockproto, "lock_nolock") == 0) {
-		/*  Nolock is always ok.  */
-	} else if (strcmp(lockproto, "lock_gulm") == 0 ||
-		   strcmp(lockproto, "lock_dlm") == 0) {
-		if (locktable == NULL || *locktable == '\0') {
+	if (strcmp(opts->lockproto, "lock_nolock") != 0 && !table_required)
+		die( _("Invalid lock protocol: %s\n"), opts->lockproto);
+
+	/* When lock_*lm is given as the lock protocol, require a lock table too */
+	if (!opts->got_locktable) {
+		if (table_required) {
 			fprintf(stderr, _("No lock table specified.\n"));
 			exit(-1);
 		}
-		for (c = locktable; *c; c++) {
-			if (!isalnum(*c) && (*c != '-') && (*c != '_') && (*c != ':'))
-				die("%s %s '%c'\n", errprefix, _("invalid character"), *c);
-		}
-
-		c = strstr(locktable, ":");
-		if (!c)
-			die("%s %s\n", errprefix, _("missing colon"));
-
-		if (c == locktable)
-			die("%s %s\n", errprefix, _("cluster name is missing"));
-		if (c - locktable > 32)
-			die("%s %s\n", errprefix, _("cluster name is too long"));
-
-		c++;
-		if (strstr(c, ":"))
-			die("%s %s\n", errprefix, _("contains more than one colon"));
-		if (!strlen(c))
-			die("%s %s\n", errprefix, _("file system name is missing"));
-		if (strlen(c) > 30)
-			die("%s %s\n", errprefix, _("file system name is too long"));
-	} else {
-		die( _("Invalid lock protocol: %s\n"), lockproto);
+		return;
+	}
+	/* User gave a lock table option, validate it */
+	if (*opts->locktable == '\0') {
+		fprintf(stderr, _("Lock table is empty.\n"));
+		exit(-1);
+	}
+	for (c = opts->locktable; *c; c++) {
+		if (!isalnum(*c) && (*c != '-') && (*c != '_') && (*c != ':'))
+			die("%s %s '%c'\n", errprefix, _("invalid character"), *c);
 	}
+	c = strstr(opts->locktable, ":");
+	if (!c)
+		die("%s %s\n", errprefix, _("missing colon"));
+
+	if (c == opts->locktable)
+		die("%s %s\n", errprefix, _("cluster name is missing"));
+	if (c - opts->locktable > 32)
+		die("%s %s\n", errprefix, _("cluster name is too long"));
+
+	c++;
+	if (strstr(c, ":"))
+		die("%s %s\n", errprefix, _("contains more than one colon"));
+	if (!strlen(c))
+		die("%s %s\n", errprefix, _("file system name is missing"));
+	if (strlen(c) > 30)
+		die("%s %s\n", errprefix, _("file system name is too long"));
 }
 
 static void are_you_sure(void)
@@ -499,7 +505,7 @@ static void opts_check(struct mkfs_opts *opts)
 		exit(1);
 	}
 
-	test_locking(opts->lockproto, opts->locktable);
+	test_locking(opts);
 
 	if (GFS2_MIN_RGSIZE > opts->rgsize || opts->rgsize > GFS2_MAX_RGSIZE)
 		/* Translators: gfs2 file systems are split into equal sized chunks called
diff --git a/tests/mkfs.at b/tests/mkfs.at
index e25b6dc..d3521c8 100644
--- a/tests/mkfs.at
+++ b/tests/mkfs.at
@@ -90,8 +90,12 @@ AT_KEYWORDS(mkfs.gfs2 mkfs)
 GFS_FSCK_CHECK([$GFS_MKFS -p lock_nolock -c 1 $GFS_TGT])
 AT_CLEANUP
 
-AT_SETUP([Incr. cluster/file system name validation])
+AT_SETUP([Lock table validation])
 AT_KEYWORDS(mkfs.gfs2 mkfs)
+AT_CHECK([$GFS_MKFS -p lock_nolock -t "" $GFS_TGT], 255, [ignore], [ignore])
+AT_CHECK([$GFS_MKFS -p lock_nolock -t "123456789012345678901234567890123:12345678" $GFS_TGT], 255, [ignore], [ignore])
+AT_CHECK([$GFS_MKFS -p lock_nolock -t "12345678901234567:1234567890123456789012345678901" $GFS_TGT], 255, [ignore], [ignore])
+GFS_FSCK_CHECK([$GFS_MKFS -p lock_nolock -t "12345678901234567890123456789012:123456789012345678901234567890" $GFS_TGT])
 AT_CHECK([$GFS_MKFS -p lock_dlm -t "" $GFS_TGT], 255, [ignore], [ignore])
 AT_CHECK([$GFS_MKFS -p lock_dlm -t "quite_long_cluster_name_test_here:intec34p" $GFS_TGT], 255, [ignore], [ignore])
 AT_CHECK([$GFS_MKFS -p lock_dlm -t "financial_cluster:this_time_we_test_fs_naming_len" $GFS_TGT], 255, [ignore], [ignore])
-- 
2.4.3

[Cluster-devel] [PATCH] mkfs.gfs2: Always validate the locktable option

[Bob Peterson]

Hi Andy,

The patch looks good. The only thing that caught my eye was that
this statement looks kinda messy and could use a few more parentheses:

(snip)
> +	if (strcmp(opts->lockproto, "lock_nolock") != 0 && !table_required)

The logic looks right, but I usually add more parens than necessary, for clarity.

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH] mkfs.gfs2: Always validate the locktable option

[Andrew Price]

Hi Bob,

On 10/12/15 13:20, Bob Peterson wrote:
> Hi Andy,
>
> The patch looks good. The only thing that caught my eye was that
> this statement looks kinda messy and could use a few more parentheses:
>
> (snip)
>> +	if (strcmp(opts->lockproto, "lock_nolock") != 0 && !table_required)
>
> The logic looks right, but I usually add more parens than necessary, for clarity.

Fair point. Changed and pushed.

Thanks!
Andy