[Cluster-devel] tunegfs2: Fix label/locktable setting code

[Cluster-devel] tunegfs2: Fix label/locktable setting code

[Steven Whitehouse]

This patch is aimed at fixing Red Hat bz #719135

The code here seemed a bit confused. I've added a check for the
format of the locktable/label based on the same code in mkfs.
In addition we set the lock proto first in order that we use the
new value of the lock proto when checking the format of
the lock table.

There is no need to have a separate function for setting the
label to the locktable, since they are both setting the same
data.

The on-disk locktable and lockproto always contain trailing
NULLs and when we read the sb from disk, we now always add NULLs
at the end of the strings so that we can rely on this later on.

The patch also checks the size of the lock table when it is
set to ensure that it is not too large.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Reported-by: Nathan Straz<nstraz@redhat.com>

diff --git a/gfs2/tune/main.c b/gfs2/tune/main.c
index 6a0daff..dc9d6a5 100644
--- a/gfs2/tune/main.c
+++ b/gfs2/tune/main.c
@@ -20,13 +20,13 @@ struct tunegfs2 tunegfs2_struct;
 struct tunegfs2 *tfs = &tunegfs2_struct;
 

-void parse_mount_options(char *arg)
+static void parse_mount_options(char *arg)
 {
 	struct opt_map *m;
 	char *s, *c;
 	int l;
 	struct opt_map {
-		char *tag;
+		const char *tag;
 		int *flag;
 		char **val;
 	} map[]= {
@@ -101,6 +101,12 @@ int main(int argc, char **argv)
 		return EX_USAGE;
 	}
 
+	if (tfs->opt_label && tfs->opt_table) {
+		fprintf(stderr, _("The -L and -o locktable= options are mutually exclusive\n"));
+		usage(argv[0]);
+		return EX_USAGE;
+	}
+
 	tfs->devicename = argv[optind];
 	tfs->fd = open(tfs->devicename, O_RDWR); 
 
@@ -120,29 +126,26 @@ int main(int argc, char **argv)
 			goto out;
 	}
 
-	/* Keep label and table together because they are the same field
-	 * in the superblock */
-
-	if (tfs->opt_label) {
-		status = change_label(tfs, tfs->label);
+	if (tfs->opt_proto) {
+		status = change_lockproto(tfs, tfs->proto);
 		if (status)
 			goto out;
 	}
 
-	if (tfs->opt_table) {
-		status = change_locktable(tfs, tfs->table);
+	if (tfs->opt_label) {
+		status = change_locktable(tfs, tfs->label);
 		if (status)
 			goto out;
 	}
 
-	if (tfs->opt_proto) {
-		status = change_lockproto(tfs, tfs->proto);
+	if (tfs->opt_table) {
+		status = change_locktable(tfs, tfs->table);
 		if (status)
 			goto out;
 	}
 
-	if (tfs->opt_label || tfs->opt_uuid ||
-			tfs->opt_table || tfs->opt_proto) {
+	if (tfs->opt_label || tfs->opt_uuid || tfs->opt_table ||
+	    tfs->opt_proto) {
 		status = write_super(tfs);
 		if (status)
 			goto out;
diff --git a/gfs2/tune/super.c b/gfs2/tune/super.c
index 9d67054..b912f11 100644
--- a/gfs2/tune/super.c
+++ b/gfs2/tune/super.c
@@ -104,6 +104,9 @@ int read_super(struct tunegfs2 *tfs)
 		fprintf(stderr, _("Not a GFS/GFS2 device\n"));
 		return EX_IOERR;
 	}
+	/* Ensure that table and proto are NULL terminated */
+	tfs->sb->sb_lockproto[GFS2_LOCKNAME_LEN - 1] = '\0';
+	tfs->sb->sb_locktable[GFS2_LOCKNAME_LEN - 1] = '\0';
 	return 0;
 }
 
@@ -115,16 +118,10 @@ static int is_gfs2(const struct tunegfs2 *tfs)
 int print_super(const struct tunegfs2 *tfs)
 {
 	char *fsname = NULL;
-	int table_len = 0, fsname_len = 0;
 
 	fsname = strchr(tfs->sb->sb_locktable, ':');
-	if (fsname) {
-		table_len = fsname - tfs->sb->sb_locktable;
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-		fsname++;
-	}
-
-	printf(_("Filesystem volume name: %.*s\n"), fsname_len, fsname);
+	if (fsname)
+		printf(_("Filesystem volume name: %s\n"), ++fsname);
 	if (is_gfs2(tfs))
 		printf(_("Filesystem UUID: %s\n"), uuid2str(tfs->sb->sb_uuid));
 	printf( _("Filesystem magic number: 0x%X\n"), be32_to_cpu(tfs->sb->sb_header.mh_magic));
@@ -133,9 +130,8 @@ int print_super(const struct tunegfs2 *tfs)
 	printf(_("Root inode: %llu\n"), (unsigned long long)be64_to_cpu(tfs->sb->sb_root_dir.no_addr));
 	if (is_gfs2(tfs))
 		printf(_("Master inode: %llu\n"), (unsigned long long)be64_to_cpu(tfs->sb->sb_master_dir.no_addr));
-	printf(_("Lock Protocol: %.*s\n"), GFS2_LOCKNAME_LEN,
-		tfs->sb->sb_lockproto);
-	printf(_("Lock table: %.*s\n"), table_len, tfs->sb->sb_locktable);
+	printf(_("Lock Protocol: %s\n"), tfs->sb->sb_lockproto);
+	printf(_("Lock table: %s\n"), tfs->sb->sb_locktable);
 
 	return 0;
 }
@@ -151,26 +147,6 @@ int write_super(const struct tunegfs2 *tfs)
 	return 0;
 }
 
-int change_label(struct tunegfs2 *tfs, const char *fsname)
-{
-	char *sb_fsname = NULL;
-	int l = strlen(fsname), table_len = 0, fsname_len = 0;
-
-	sb_fsname = strchr(tfs->sb->sb_locktable, ':');
-	if (sb_fsname) {
-		table_len = sb_fsname - tfs->sb->sb_locktable;
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-		sb_fsname++;
-	}
-	if (fsname_len < l) {
-		fprintf(stderr, _("Label too long\n"));
-		return EX_DATAERR;
-	}
-	memset(sb_fsname, '\0', fsname_len);
-	memcpy(sb_fsname, fsname, l);
-	return 0;
-}
-
 int change_uuid(struct tunegfs2 *tfs, const char *str)
 {
 	char uuid[16];
@@ -202,30 +178,28 @@ int change_lockproto(struct tunegfs2 *tfs, const char *lockproto)
 
 int change_locktable(struct tunegfs2 *tfs, const char *locktable)
 {
-	char *sb_fsname = NULL;
-	char t_fsname[GFS2_LOCKNAME_LEN];
-	int l = strlen(locktable), table_len = 0, fsname_len = 0;
-
-	sb_fsname = strchr(tfs->sb->sb_locktable, ':');
-	if (sb_fsname) {
-		table_len = sb_fsname - tfs->sb->sb_locktable;
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-		sb_fsname++;
-	}
-	/* Gotta check if the existing fsname will allow us to fit in
-	 * the new locktable name */
-	fsname_len = strlen(sb_fsname);
-	if (fsname_len > GFS2_LOCKNAME_LEN - table_len - 1)
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-
-	if (l > GFS2_LOCKNAME_LEN - fsname_len - 1) {
-		fprintf(stderr, _("Lock table name too big\n"));
+	if (strlen(locktable) >= GFS2_LOCKNAME_LEN ) {
+		fprintf(stderr, _("Lock table name too long\n"));
 		return EX_DATAERR;
 	}
-	memset(t_fsname, '\0', GFS2_LOCKNAME_LEN);
-	strncpy(t_fsname, sb_fsname, fsname_len);
-	memset(tfs->sb->sb_locktable, '\0', GFS2_LOCKNAME_LEN);
-	sprintf(tfs->sb->sb_locktable, "%s:%s", locktable, t_fsname);
+
+	if (strcmp(tfs->sb->sb_lockproto, "lock_dlm") == 0) {
+		char *fsname = strchr(locktable, ':');
+		if (fsname == NULL) {
+			fprintf(stderr, _("locktable error: mising colon in the locktable\n"));
+			return EX_DATAERR;
+		}
+		if (strlen(++fsname) > 16) {
+			fprintf(stderr, _("locktable error: fsname too long\n"));
+			return EX_DATAERR;
+		}
+		if (strchr(fsname, ':')) {
+			fprintf(stderr, _("locktable error: more than one colon present\n"));
+			return EX_DATAERR;
+		}
+	}
+
+	strcpy(tfs->sb->sb_locktable, locktable);
 	return 0;
 }
 
diff --git a/gfs2/tune/tunegfs2.h b/gfs2/tune/tunegfs2.h
index 8fe7e07..3b28c58 100644
--- a/gfs2/tune/tunegfs2.h
+++ b/gfs2/tune/tunegfs2.h
@@ -24,7 +24,6 @@ extern int print_super(const struct tunegfs2 *);
 extern int read_super(struct tunegfs2 *);
 extern int write_super(const struct tunegfs2 *);
 extern int change_uuid(struct tunegfs2 *, const char *uuid);
-extern int change_label(struct tunegfs2 *, const char *label);
 extern int change_lockproto(struct tunegfs2 *, const char *label);
 extern int change_locktable(struct tunegfs2 *, const char *label);
 

[Cluster-devel] tunegfs2: Fix label/locktable setting code

[Steven Whitehouse]

Hi,

On Thu, 2011-07-21 at 12:00 +0100, Andrew Price wrote:
> Hi Steve,
> 
> On 20/07/11 10:57, Steven Whitehouse wrote:
> >
> > This is an updated version of the previously posted patch aimed at
> > fixing bz #719135
> 
> Looks good to me. Just a couple of minor notes on input validation...
> 
> With this patch it's possible to set the locktable to ":" using:
> 
> # tunegfs2 -o lockproto=lock_dlm /dev/dm-3
> # tunegfs2 -L : /dev/dm-3
> 
> And although this rightly fails:
> 
> # tunegfs2 -o lockproto=lock_dlm,locktable=foo /dev/dm-3
> locktable error: mising colon in the locktable
> 
> it is still possible to specify a colon-less locktable name with lock_dlm:
> 
> # tunegfs2 -o lockproto=lock_nolock /dev/dm-3
> # tunegfs2 -L foo /dev/dm-3
> # tunegfs2 -o lockproto=lock_dlm /dev/dm-3
> 
Yes, I'm not overly worried by that. Mounting will fail so it is still
impossible to cause a problem by doing that, I think,

Steve.

[Cluster-devel] tunegfs2: Fix label/locktable setting code

[Andrew Price]

Hi Steve,

On 20/07/11 10:57, Steven Whitehouse wrote:
>
> This is an updated version of the previously posted patch aimed at
> fixing bz #719135

Looks good to me. Just a couple of minor notes on input validation...

With this patch it's possible to set the locktable to ":" using:

# tunegfs2 -o lockproto=lock_dlm /dev/dm-3
# tunegfs2 -L : /dev/dm-3

And although this rightly fails:

# tunegfs2 -o lockproto=lock_dlm,locktable=foo /dev/dm-3
locktable error: mising colon in the locktable

it is still possible to specify a colon-less locktable name with lock_dlm:

# tunegfs2 -o lockproto=lock_nolock /dev/dm-3
# tunegfs2 -L foo /dev/dm-3
# tunegfs2 -o lockproto=lock_dlm /dev/dm-3

> The differences from the previous patch are that the printing function
> now prints the whole of the label, to match the setting function and
> it also checks the size of the lock protocol string, which was
> previously not done.
>
> The code here seemed a bit confused. I've added a check for the
> format of the locktable/label based on the same code in mkfs.
> In addition we set the lock proto first in order that we use the
> new value of the lock proto when checking the format of
> the lock table.
>
> There is no need to have a separate function for setting the
> label to the locktable, since they are both setting the same
> data.
>
> The on-disk locktable and lockproto always contain trailing
> NULLs and when we read the sb from disk, we now always add NULLs
> at the end of the strings so that we can rely on this later on.
>
> The patch also checks the size of the lock table when it is
> set to ensure that it is not too large.
>
> Signed-off-by: Steven Whitehouse<swhiteho@redhat.com>
> Reported-by: Nathan Straz<nstraz@redhat.com>
>
> diff --git a/gfs2/tune/main.c b/gfs2/tune/main.c
> index 6a0daff..dc9d6a5 100644
> --- a/gfs2/tune/main.c
> +++ b/gfs2/tune/main.c
> @@ -20,13 +20,13 @@ struct tunegfs2 tunegfs2_struct;
>   struct tunegfs2 *tfs =&tunegfs2_struct;
>
>
> -void parse_mount_options(char *arg)
> +static void parse_mount_options(char *arg)
>   {
>   	struct opt_map *m;
>   	char *s, *c;
>   	int l;
>   	struct opt_map {
> -		char *tag;
> +		const char *tag;
>   		int *flag;
>   		char **val;
>   	} map[]= {
> @@ -101,6 +101,12 @@ int main(int argc, char **argv)
>   		return EX_USAGE;
>   	}
>
> +	if (tfs->opt_label&&  tfs->opt_table) {
> +		fprintf(stderr, _("The -L and -o locktable= options are mutually exclusive\n"));
> +		usage(argv[0]);
> +		return EX_USAGE;
> +	}
> +
>   	tfs->devicename = argv[optind];
>   	tfs->fd = open(tfs->devicename, O_RDWR);
>
> @@ -120,29 +126,26 @@ int main(int argc, char **argv)
>   			goto out;
>   	}
>
> -	/* Keep label and table together because they are the same field
> -	 * in the superblock */
> -
> -	if (tfs->opt_label) {
> -		status = change_label(tfs, tfs->label);
> +	if (tfs->opt_proto) {
> +		status = change_lockproto(tfs, tfs->proto);
>   		if (status)
>   			goto out;
>   	}
>
> -	if (tfs->opt_table) {
> -		status = change_locktable(tfs, tfs->table);
> +	if (tfs->opt_label) {
> +		status = change_locktable(tfs, tfs->label);
>   		if (status)
>   			goto out;
>   	}
>
> -	if (tfs->opt_proto) {
> -		status = change_lockproto(tfs, tfs->proto);
> +	if (tfs->opt_table) {
> +		status = change_locktable(tfs, tfs->table);
>   		if (status)
>   			goto out;
>   	}
>
> -	if (tfs->opt_label || tfs->opt_uuid ||
> -			tfs->opt_table || tfs->opt_proto) {
> +	if (tfs->opt_label || tfs->opt_uuid || tfs->opt_table ||
> +	    tfs->opt_proto) {
>   		status = write_super(tfs);
>   		if (status)
>   			goto out;
> diff --git a/gfs2/tune/super.c b/gfs2/tune/super.c
> index 9d67054..65e8d5b 100644
> --- a/gfs2/tune/super.c
> +++ b/gfs2/tune/super.c
> @@ -104,6 +104,9 @@ int read_super(struct tunegfs2 *tfs)
>   		fprintf(stderr, _("Not a GFS/GFS2 device\n"));
>   		return EX_IOERR;
>   	}
> +	/* Ensure that table and proto are NULL terminated */
> +	tfs->sb->sb_lockproto[GFS2_LOCKNAME_LEN - 1] = '\0';
> +	tfs->sb->sb_locktable[GFS2_LOCKNAME_LEN - 1] = '\0';
>   	return 0;
>   }
>
> @@ -114,17 +117,7 @@ static int is_gfs2(const struct tunegfs2 *tfs)
>
>   int print_super(const struct tunegfs2 *tfs)
>   {
> -	char *fsname = NULL;
> -	int table_len = 0, fsname_len = 0;
> -
> -	fsname = strchr(tfs->sb->sb_locktable, ':');
> -	if (fsname) {
> -		table_len = fsname - tfs->sb->sb_locktable;
> -		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
> -		fsname++;
> -	}
> -
> -	printf(_("Filesystem volume name: %.*s\n"), fsname_len, fsname);
> +	printf(_("Filesystem volume name: %s\n"), tfs->sb->sb_locktable);
>   	if (is_gfs2(tfs))
>   		printf(_("Filesystem UUID: %s\n"), uuid2str(tfs->sb->sb_uuid));
>   	printf( _("Filesystem magic number: 0x%X\n"), be32_to_cpu(tfs->sb->sb_header.mh_magic));
> @@ -133,9 +126,8 @@ int print_super(const struct tunegfs2 *tfs)
>   	printf(_("Root inode: %llu\n"), (unsigned long long)be64_to_cpu(tfs->sb->sb_root_dir.no_addr));
>   	if (is_gfs2(tfs))
>   		printf(_("Master inode: %llu\n"), (unsigned long long)be64_to_cpu(tfs->sb->sb_master_dir.no_addr));
> -	printf(_("Lock Protocol: %.*s\n"), GFS2_LOCKNAME_LEN,
> -		tfs->sb->sb_lockproto);
> -	printf(_("Lock table: %.*s\n"), table_len, tfs->sb->sb_locktable);
> +	printf(_("Lock Protocol: %s\n"), tfs->sb->sb_lockproto);
> +	printf(_("Lock table: %s\n"), tfs->sb->sb_locktable);
>
>   	return 0;
>   }
> @@ -151,26 +143,6 @@ int write_super(const struct tunegfs2 *tfs)
>   	return 0;
>   }
>
> -int change_label(struct tunegfs2 *tfs, const char *fsname)
> -{
> -	char *sb_fsname = NULL;
> -	int l = strlen(fsname), table_len = 0, fsname_len = 0;
> -
> -	sb_fsname = strchr(tfs->sb->sb_locktable, ':');
> -	if (sb_fsname) {
> -		table_len = sb_fsname - tfs->sb->sb_locktable;
> -		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
> -		sb_fsname++;
> -	}
> -	if (fsname_len<  l) {
> -		fprintf(stderr, _("Label too long\n"));
> -		return EX_DATAERR;
> -	}
> -	memset(sb_fsname, '\0', fsname_len);
> -	memcpy(sb_fsname, fsname, l);
> -	return 0;
> -}
> -
>   int change_uuid(struct tunegfs2 *tfs, const char *str)
>   {
>   	char uuid[16];
> @@ -186,12 +158,17 @@ int change_uuid(struct tunegfs2 *tfs, const char *str)
>   	return status;
>   }
>
> -
>   int change_lockproto(struct tunegfs2 *tfs, const char *lockproto)
>   {
>   	int l = strlen(lockproto);
> -	if (strncmp(lockproto, "lock_dlm", 8)
> -			&&  strncmp(lockproto, "lock_nolock", 11)) {
> +
> +	if (l>= GFS2_LOCKNAME_LEN) {
> +		fprintf(stderr, _("Lock protocol name too long\n"));
> +		return EX_DATAERR;
> +	}
> +
> +	if (strncmp(lockproto, "lock_dlm", 8)&&
> +	    strncmp(lockproto, "lock_nolock", 11)) {
>   		fprintf(stderr, _("Incorrect lock protocol specified\n"));
>   		return EX_DATAERR;
>   	}
> @@ -202,30 +179,28 @@ int change_lockproto(struct tunegfs2 *tfs, const char *lockproto)
>
>   int change_locktable(struct tunegfs2 *tfs, const char *locktable)
>   {
> -	char *sb_fsname = NULL;
> -	char t_fsname[GFS2_LOCKNAME_LEN];
> -	int l = strlen(locktable), table_len = 0, fsname_len = 0;
> -
> -	sb_fsname = strchr(tfs->sb->sb_locktable, ':');
> -	if (sb_fsname) {
> -		table_len = sb_fsname - tfs->sb->sb_locktable;
> -		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
> -		sb_fsname++;
> -	}
> -	/* Gotta check if the existing fsname will allow us to fit in
> -	 * the new locktable name */
> -	fsname_len = strlen(sb_fsname);
> -	if (fsname_len>  GFS2_LOCKNAME_LEN - table_len - 1)
> -		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
> -
> -	if (l>  GFS2_LOCKNAME_LEN - fsname_len - 1) {
> -		fprintf(stderr, _("Lock table name too big\n"));
> +	if (strlen(locktable)>= GFS2_LOCKNAME_LEN) {
> +		fprintf(stderr, _("Lock table name too long\n"));
>   		return EX_DATAERR;
>   	}
> -	memset(t_fsname, '\0', GFS2_LOCKNAME_LEN);
> -	strncpy(t_fsname, sb_fsname, fsname_len);
> -	memset(tfs->sb->sb_locktable, '\0', GFS2_LOCKNAME_LEN);
> -	sprintf(tfs->sb->sb_locktable, "%s:%s", locktable, t_fsname);
> +
> +	if (strcmp(tfs->sb->sb_lockproto, "lock_dlm") == 0) {
> +		char *fsname = strchr(locktable, ':');
> +		if (fsname == NULL) {
> +			fprintf(stderr, _("locktable error: mising colon in the locktable\n"));
> +			return EX_DATAERR;
> +		}
> +		if (strlen(++fsname)>  16) {
> +			fprintf(stderr, _("locktable error: fsname too long\n"));
> +			return EX_DATAERR;
> +		}
> +		if (strchr(fsname, ':')) {
> +			fprintf(stderr, _("locktable error: more than one colon present\n"));
> +			return EX_DATAERR;
> +		}
> +	}
> +
> +	strcpy(tfs->sb->sb_locktable, locktable);
>   	return 0;
>   }
>
> diff --git a/gfs2/tune/tunegfs2.h b/gfs2/tune/tunegfs2.h
> index 8fe7e07..3b28c58 100644
> --- a/gfs2/tune/tunegfs2.h
> +++ b/gfs2/tune/tunegfs2.h
> @@ -24,7 +24,6 @@ extern int print_super(const struct tunegfs2 *);
>   extern int read_super(struct tunegfs2 *);
>   extern int write_super(const struct tunegfs2 *);
>   extern int change_uuid(struct tunegfs2 *, const char *uuid);
> -extern int change_label(struct tunegfs2 *, const char *label);
>   extern int change_lockproto(struct tunegfs2 *, const char *label);
>   extern int change_locktable(struct tunegfs2 *, const char *label);
>
>
>

[Cluster-devel] tunegfs2: Fix label/locktable setting code

[Steven Whitehouse]

This is an updated version of the previously posted patch aimed at
fixing bz #719135

The differences from the previous patch are that the printing function
now prints the whole of the label, to match the setting function and
it also checks the size of the lock protocol string, which was
previously not done.

The code here seemed a bit confused. I've added a check for the
format of the locktable/label based on the same code in mkfs.
In addition we set the lock proto first in order that we use the
new value of the lock proto when checking the format of
the lock table.

There is no need to have a separate function for setting the
label to the locktable, since they are both setting the same
data.

The on-disk locktable and lockproto always contain trailing
NULLs and when we read the sb from disk, we now always add NULLs
at the end of the strings so that we can rely on this later on.

The patch also checks the size of the lock table when it is
set to ensure that it is not too large.

Signed-off-by: Steven Whitehouse <swhiteho@redhat.com>
Reported-by: Nathan Straz <nstraz@redhat.com>

diff --git a/gfs2/tune/main.c b/gfs2/tune/main.c
index 6a0daff..dc9d6a5 100644
--- a/gfs2/tune/main.c
+++ b/gfs2/tune/main.c
@@ -20,13 +20,13 @@ struct tunegfs2 tunegfs2_struct;
 struct tunegfs2 *tfs = &tunegfs2_struct;
 
 
-void parse_mount_options(char *arg)
+static void parse_mount_options(char *arg)
 {
 	struct opt_map *m;
 	char *s, *c;
 	int l;
 	struct opt_map {
-		char *tag;
+		const char *tag;
 		int *flag;
 		char **val;
 	} map[]= {
@@ -101,6 +101,12 @@ int main(int argc, char **argv)
 		return EX_USAGE;
 	}
 
+	if (tfs->opt_label && tfs->opt_table) {
+		fprintf(stderr, _("The -L and -o locktable= options are mutually exclusive\n"));
+		usage(argv[0]);
+		return EX_USAGE;
+	}
+
 	tfs->devicename = argv[optind];
 	tfs->fd = open(tfs->devicename, O_RDWR); 
 
@@ -120,29 +126,26 @@ int main(int argc, char **argv)
 			goto out;
 	}
 
-	/* Keep label and table together because they are the same field
-	 * in the superblock */
-
-	if (tfs->opt_label) {
-		status = change_label(tfs, tfs->label);
+	if (tfs->opt_proto) {
+		status = change_lockproto(tfs, tfs->proto);
 		if (status)
 			goto out;
 	}
 
-	if (tfs->opt_table) {
-		status = change_locktable(tfs, tfs->table);
+	if (tfs->opt_label) {
+		status = change_locktable(tfs, tfs->label);
 		if (status)
 			goto out;
 	}
 
-	if (tfs->opt_proto) {
-		status = change_lockproto(tfs, tfs->proto);
+	if (tfs->opt_table) {
+		status = change_locktable(tfs, tfs->table);
 		if (status)
 			goto out;
 	}
 
-	if (tfs->opt_label || tfs->opt_uuid ||
-			tfs->opt_table || tfs->opt_proto) {
+	if (tfs->opt_label || tfs->opt_uuid || tfs->opt_table ||
+	    tfs->opt_proto) {
 		status = write_super(tfs);
 		if (status)
 			goto out;
diff --git a/gfs2/tune/super.c b/gfs2/tune/super.c
index 9d67054..65e8d5b 100644
--- a/gfs2/tune/super.c
+++ b/gfs2/tune/super.c
@@ -104,6 +104,9 @@ int read_super(struct tunegfs2 *tfs)
 		fprintf(stderr, _("Not a GFS/GFS2 device\n"));
 		return EX_IOERR;
 	}
+	/* Ensure that table and proto are NULL terminated */
+	tfs->sb->sb_lockproto[GFS2_LOCKNAME_LEN - 1] = '\0';
+	tfs->sb->sb_locktable[GFS2_LOCKNAME_LEN - 1] = '\0';
 	return 0;
 }
 
@@ -114,17 +117,7 @@ static int is_gfs2(const struct tunegfs2 *tfs)
 
 int print_super(const struct tunegfs2 *tfs)
 {
-	char *fsname = NULL;
-	int table_len = 0, fsname_len = 0;
-
-	fsname = strchr(tfs->sb->sb_locktable, ':');
-	if (fsname) {
-		table_len = fsname - tfs->sb->sb_locktable;
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-		fsname++;
-	}
-
-	printf(_("Filesystem volume name: %.*s\n"), fsname_len, fsname);
+	printf(_("Filesystem volume name: %s\n"), tfs->sb->sb_locktable);
 	if (is_gfs2(tfs))
 		printf(_("Filesystem UUID: %s\n"), uuid2str(tfs->sb->sb_uuid));
 	printf( _("Filesystem magic number: 0x%X\n"), be32_to_cpu(tfs->sb->sb_header.mh_magic));
@@ -133,9 +126,8 @@ int print_super(const struct tunegfs2 *tfs)
 	printf(_("Root inode: %llu\n"), (unsigned long long)be64_to_cpu(tfs->sb->sb_root_dir.no_addr));
 	if (is_gfs2(tfs))
 		printf(_("Master inode: %llu\n"), (unsigned long long)be64_to_cpu(tfs->sb->sb_master_dir.no_addr));
-	printf(_("Lock Protocol: %.*s\n"), GFS2_LOCKNAME_LEN,
-		tfs->sb->sb_lockproto);
-	printf(_("Lock table: %.*s\n"), table_len, tfs->sb->sb_locktable);
+	printf(_("Lock Protocol: %s\n"), tfs->sb->sb_lockproto);
+	printf(_("Lock table: %s\n"), tfs->sb->sb_locktable);
 
 	return 0;
 }
@@ -151,26 +143,6 @@ int write_super(const struct tunegfs2 *tfs)
 	return 0;
 }
 
-int change_label(struct tunegfs2 *tfs, const char *fsname)
-{
-	char *sb_fsname = NULL;
-	int l = strlen(fsname), table_len = 0, fsname_len = 0;
-
-	sb_fsname = strchr(tfs->sb->sb_locktable, ':');
-	if (sb_fsname) {
-		table_len = sb_fsname - tfs->sb->sb_locktable;
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-		sb_fsname++;
-	}
-	if (fsname_len < l) {
-		fprintf(stderr, _("Label too long\n"));
-		return EX_DATAERR;
-	}
-	memset(sb_fsname, '\0', fsname_len);
-	memcpy(sb_fsname, fsname, l);
-	return 0;
-}
-
 int change_uuid(struct tunegfs2 *tfs, const char *str)
 {
 	char uuid[16];
@@ -186,12 +158,17 @@ int change_uuid(struct tunegfs2 *tfs, const char *str)
 	return status;
 }
 
-
 int change_lockproto(struct tunegfs2 *tfs, const char *lockproto)
 {
 	int l = strlen(lockproto);
-	if (strncmp(lockproto, "lock_dlm", 8) 
-			&& strncmp(lockproto, "lock_nolock", 11)) {
+
+	if (l >= GFS2_LOCKNAME_LEN) {
+		fprintf(stderr, _("Lock protocol name too long\n"));
+		return EX_DATAERR;
+	}
+
+	if (strncmp(lockproto, "lock_dlm", 8) &&
+	    strncmp(lockproto, "lock_nolock", 11)) {
 		fprintf(stderr, _("Incorrect lock protocol specified\n"));
 		return EX_DATAERR;
 	}
@@ -202,30 +179,28 @@ int change_lockproto(struct tunegfs2 *tfs, const char *lockproto)
 
 int change_locktable(struct tunegfs2 *tfs, const char *locktable)
 {
-	char *sb_fsname = NULL;
-	char t_fsname[GFS2_LOCKNAME_LEN];
-	int l = strlen(locktable), table_len = 0, fsname_len = 0;
-
-	sb_fsname = strchr(tfs->sb->sb_locktable, ':');
-	if (sb_fsname) {
-		table_len = sb_fsname - tfs->sb->sb_locktable;
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-		sb_fsname++;
-	}
-	/* Gotta check if the existing fsname will allow us to fit in
-	 * the new locktable name */
-	fsname_len = strlen(sb_fsname);
-	if (fsname_len > GFS2_LOCKNAME_LEN - table_len - 1)
-		fsname_len = GFS2_LOCKNAME_LEN - table_len - 1;
-
-	if (l > GFS2_LOCKNAME_LEN - fsname_len - 1) {
-		fprintf(stderr, _("Lock table name too big\n"));
+	if (strlen(locktable) >= GFS2_LOCKNAME_LEN) {
+		fprintf(stderr, _("Lock table name too long\n"));
 		return EX_DATAERR;
 	}
-	memset(t_fsname, '\0', GFS2_LOCKNAME_LEN);
-	strncpy(t_fsname, sb_fsname, fsname_len);
-	memset(tfs->sb->sb_locktable, '\0', GFS2_LOCKNAME_LEN);
-	sprintf(tfs->sb->sb_locktable, "%s:%s", locktable, t_fsname);
+
+	if (strcmp(tfs->sb->sb_lockproto, "lock_dlm") == 0) {
+		char *fsname = strchr(locktable, ':');
+		if (fsname == NULL) {
+			fprintf(stderr, _("locktable error: mising colon in the locktable\n"));
+			return EX_DATAERR;
+		}
+		if (strlen(++fsname) > 16) {
+			fprintf(stderr, _("locktable error: fsname too long\n"));
+			return EX_DATAERR;
+		}
+		if (strchr(fsname, ':')) {
+			fprintf(stderr, _("locktable error: more than one colon present\n"));
+			return EX_DATAERR;
+		}
+	}
+
+	strcpy(tfs->sb->sb_locktable, locktable);
 	return 0;
 }
 
diff --git a/gfs2/tune/tunegfs2.h b/gfs2/tune/tunegfs2.h
index 8fe7e07..3b28c58 100644
--- a/gfs2/tune/tunegfs2.h
+++ b/gfs2/tune/tunegfs2.h
@@ -24,7 +24,6 @@ extern int print_super(const struct tunegfs2 *);
 extern int read_super(struct tunegfs2 *);
 extern int write_super(const struct tunegfs2 *);
 extern int change_uuid(struct tunegfs2 *, const char *uuid);
-extern int change_label(struct tunegfs2 *, const char *label);
 extern int change_lockproto(struct tunegfs2 *, const char *label);
 extern int change_locktable(struct tunegfs2 *, const char *label);