mkfs.gfs2: Always validate the locktable option
PATCH 1/1 mkfs.gfs2: Always validate the locktable option
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.
--- gfs2/mkfs/main_mkfs.c | 66 ++++++++++++++++++++++++++++----------------------- tests/mkfs.at | 6 ++++- 2 files changed, 41 insertions(+), 31 deletions(-)
gfs2/mkfs/main_mkfs.c +36 -30
@@ -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
tests/mkfs.at +5 -1
@@ -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])
Signature · 1 line
2.4.3
PATCH 1/1 mkfs.gfs2: Always validate the locktable option
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)
gfs2/mkfs/main_mkfs.c static void opts_get(int argc, char *argv[], struct mkfs_opts *opts) · line 385
Quoted 1 line
gfs2/mkfs/main_mkfs.c +1
-0+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
PATCH 1/1 mkfs.gfs2: Always validate the locktable option
Hi Bob,
Quoted 9 lines · 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)
gfs2/mkfs/main_mkfs.c static void opts_get(int argc, char *argv[], struct mkfs_opts *opts) · line 385
Quoted 1 line
gfs2/mkfs/main_mkfs.c +1
-0+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