From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Price <anprice@redhat.com>
Date: Tue, 21 Apr 2015 18:01:15 +0100
Subject: [Cluster-devel] [PATCH] mkfs.gfs2: Allow longer cluster names
In-Reply-To: <1429631567-18662-1-git-send-email-pevans@redhat.com>
References: <1429631567-18662-1-git-send-email-pevans@redhat.com>
Message-ID: <5536825B.60502@redhat.com>
List-Id: <cluster-devel.redhat.com>
To: cluster-devel.redhat.com
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit

Hi Paul,

On 21/04/15 16:52, Paul Evans wrote:
> Increase the enforced limit for cluster name to 32 bytes and file
> system name to 30 bytes for mkfs.gfs2 (was previously 16 + 16
> bytes).
>
> Also increased this limit in tunegfs2 when relabling gfs2 file
> sytems.
>
> Updated the formation in the man pages along with adding a new test
> case for mkfs.gfs2 to validate the increased cluster/file system
> name support.

Could you add a signed-off-by? git commit --amend -s will do that quickly.

> ---
>   gfs2/man/mkfs.gfs2.8  | 2 +-
>   gfs2/mkfs/main_mkfs.c | 4 ++--
>   gfs2/tune/super.c     | 2 +-
>   tests/mkfs.at         | 6 ++++++
>   4 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/gfs2/man/mkfs.gfs2.8 b/gfs2/man/mkfs.gfs2.8
> index ceb6f38..44483f1 100644
> --- a/gfs2/man/mkfs.gfs2.8
> +++ b/gfs2/man/mkfs.gfs2.8
> @@ -103,7 +103,7 @@ It is \fIclustername:fsname\fR.
>   Clustername must match that in cluster.conf; only members of this
>   cluster are permitted to use this file system.
>   Fsname is a unique file system name used to distinguish this GFS2 file
> -system from others created (1 to 16 characters).  Lock_nolock doesn't
> +system from others created (1 to 30 characters).  Lock_nolock doesn't
>   use this field. Valid \fIclustername\fRs and \fIfsname\fRs may only contain
>   alphanumeric characters, hyphens (-) and underscores (_).

Hm, the man page doesn't seem to mention the cluster name length limit, 
maybe we should add that.

>   .TP
> diff --git a/gfs2/mkfs/main_mkfs.c b/gfs2/mkfs/main_mkfs.c
> index 0636f0b..3fab08c 100644
> --- a/gfs2/mkfs/main_mkfs.c
> +++ b/gfs2/mkfs/main_mkfs.c
> @@ -398,7 +398,7 @@ static void test_locking(const char *lockproto, const char *locktable)
>
>   		if (c == locktable)
>   			die("%s %s\n", errprefix, _("cluster name is missing"));
> -		if (c - locktable > 16)
> +		if (c - locktable > 32)
>   			die("%s %s\n", errprefix, _("cluster name is too long"));
>
>   		c++;
> @@ -406,7 +406,7 @@ static void test_locking(const char *lockproto, const char *locktable)
>   			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) > 16)
> +		if (strlen(c) > 30)
>   			die("%s %s\n", errprefix, _("file system name is too long"));
>   	} else {
>   		die( _("Invalid lock protocol: %s\n"), lockproto);
> diff --git a/gfs2/tune/super.c b/gfs2/tune/super.c
> index cbd0026..560ce68 100644
> --- a/gfs2/tune/super.c
> +++ b/gfs2/tune/super.c
> @@ -194,7 +194,7 @@ int change_locktable(struct tunegfs2 *tfs, const char *locktable)
>   			fprintf(stderr, "%s %s\n", errpre, _("missing colon"));
>   			return EX_DATAERR;
>   		}
> -		if (strlen(++fsname) > 16) {
> +		if (strlen(++fsname) > 30) {
>   			fprintf(stderr, "%s %s\n", errpre, _("file system name is too long"));
>   			return EX_DATAERR;
>   		}
> diff --git a/tests/mkfs.at b/tests/mkfs.at
> index 438184c..786f30c 100644
> --- a/tests/mkfs.at
> +++ b/tests/mkfs.at
> @@ -89,3 +89,9 @@ AT_SETUP([Min. quota change file size])
>   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 support])
> +AT_KEYWORDS(mkfs.gfs2 mkfs)
> +GFS_FSCK_CHECK([$GFS_MKFS -p lock_dlm -t "financial_cluster:intec34p" $GFS_TGT])
> +GFS_FSCK_CHECK([$GFS_MKFS -p lock_dlm -t "financial_cluster:financial_volume-001" $GFS_TGT])
> +AT_CLEANUP

Thanks for adding tests! Ideally they should exercise the validation 
code that you've changed, so boundary and invalid values would be more 
effective. Could you change these into tests for, e.g.:

Zero-length locktable -> expect mkfs failure (no need to fsck)
Max fsname length, clustername 1 char too long -> ditto
Max clustername length, fsname 1 char too long -> ditto
Max fsname and clustername length -> expect success (and fsck success)

These kind of tests seem too trivial to be useful when you're writing 
them but I assure you they will catch silly regressions next time 
somebody decides to rework that code :)

Cheers,
Andy