Adds a utility to get/set uuid through ext4 ioctl. Executes the ioctls while
running fsstress. These ioctls are used by tune2fs to safely change the uuid
without racing other filesystem modifications.
Signed-off-by: Jeremy Bongio <[email protected]>
---
src/Makefile | 4 +-
src/global.h | 2 +
src/uuid_ioctl.c | 106 +++++++++++++++++++++++++++++++++++++++++++++
tests/ext4/056 | 59 +++++++++++++++++++++++++
tests/ext4/056.out | 1 +
5 files changed, 170 insertions(+), 2 deletions(-)
create mode 100644 src/uuid_ioctl.c
create mode 100755 tests/ext4/056
create mode 100644 tests/ext4/056.out
diff --git a/src/Makefile b/src/Makefile
index 111ce1d9..e33e04de 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -31,14 +31,14 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
dio-invalidate-cache stat_test t_encrypted_d_revalidate \
attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \
fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \
- detached_mounts_propagation ext4_resize
+ detached_mounts_propagation ext4_resize uuid_ioctl
EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \
btrfs_crc32c_forged_name.py
SUBDIRS = log-writes perf
-LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt
+LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt -luuid
ifeq ($(HAVE_XLOG_ASSIGN_LSN), true)
LINUX_TARGETS += loggen
diff --git a/src/global.h b/src/global.h
index b4407099..747d95f8 100644
--- a/src/global.h
+++ b/src/global.h
@@ -184,4 +184,6 @@ roundup_64(unsigned long long x, unsigned int y)
return rounddown_64(x + y - 1, y);
}
+#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
+
#endif /* GLOBAL_H */
diff --git a/src/uuid_ioctl.c b/src/uuid_ioctl.c
new file mode 100644
index 00000000..51480689
--- /dev/null
+++ b/src/uuid_ioctl.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Test program which uses the raw ext4 set_fsuuid ioctl directly.
+ * SYNOPSIS:
+ * $0 COMMAND MOUNT_POINT [UUID]
+ *
+ * COMMAND must be either "get" or "set".
+ * The UUID must be a 16 octet represented as 32 hexadecimal digits in canonical
+ * textual representation, e.g. output from `uuidgen`.
+ *
+ */
+
+#include <stdio.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <unistd.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+#include <uuid/uuid.h>
+#include <linux/fs.h>
+#include "global.h"
+
+struct fsuuid {
+ size_t len;
+ __u8 *b;
+};
+
+#ifndef EXT4_IOC_GETFSUUID
+#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid)
+#endif
+
+#ifndef EXT4_IOC_SETFSUUID
+#define EXT4_IOC_SETFSUUID _IOW('f', 45, struct fsuuid)
+#endif
+
+int main(int argc, char **argv)
+{
+ int error, fd;
+ struct fsuuid fsuuid;
+
+ if (argc < 3) {
+ fprintf(stderr, "Invalid arguments\n");
+ return 1;
+ }
+
+ fd = open(argv[2], O_RDONLY);
+ if (!fd) {
+ perror(argv[2]);
+ return 1;
+ }
+
+ BUILD_BUG_ON(sizeof(uuid_t) % 16);
+ fsuuid.len = 16;
+ fsuuid.b = calloc(fsuuid.len, sizeof(__u8));
+
+ if (strcmp(argv[1], "get") == 0) {
+ uuid_t uuid;
+ char uuid_str[37];
+
+ if (ioctl(fd, EXT4_IOC_GETFSUUID, &fsuuid)) {
+ close(fd);
+ fprintf(stderr, "%s while trying to get fs uuid\n",
+ strerror(errno));
+ return 1;
+ }
+
+ memcpy(&uuid, fsuuid.b, sizeof(uuid));
+ uuid_unparse(uuid, uuid_str);
+ printf("%s", uuid_str);
+ } else if (strcmp(argv[1], "set") == 0) {
+ if (argc != 4) {
+ fprintf(stderr, "UUID argument missing.\n");
+ return 1;
+ }
+
+ if (strlen(argv[3]) != 36) {
+ fprintf(stderr, "Invalid UUID. The UUID should be in "
+ "canonical format. Example: "
+ "8c628557-6987-42b2-ba16-b7cc79ddfb43\n");
+ return 1;
+ }
+
+ uuid_t uuid;
+ error = uuid_parse(argv[3], uuid);
+ if (error < 0) {
+ fprintf(stderr, "%s: invalid UUID.\n", argv[0]);
+ return 1;
+ }
+
+ memcpy(fsuuid.b, uuid, sizeof(uuid));
+ if(ioctl(fd, EXT4_IOC_SETFSUUID, &fsuuid)) {
+ close(fd);
+ fprintf(stderr, "%s while trying to set fs uuid\n",
+ strerror(errno));
+ return 1;
+ }
+ } else {
+ fprintf(stderr, "Invalid command\n");
+ }
+
+ close(fd);
+ return 0;
+}
diff --git a/tests/ext4/056 b/tests/ext4/056
new file mode 100755
index 00000000..46631d46
--- /dev/null
+++ b/tests/ext4/056
@@ -0,0 +1,59 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2000-2005 Silicon Graphics, Inc. All Rights Reserved.
+#
+# Test the set/get UUID ioctl.
+#
+
+. ./common/preamble
+_begin_fstest auto ioctl resize
+
+tmpfile="/tmp/$$."
+trap "rm -f $tmpfile; exit" 0 1 2 3 15
+
+# Import common functions.
+. ./common/filter
+
+# real QA test starts here
+_supported_fs ext4
+_require_scratch
+_require_test_program uuid_ioctl
+
+UUID_IOCTL=$here/src/uuid_ioctl
+
+# if the ioctl is not supported by the kernel, then skip test.
+current_uid=$($UUID_IOCTL get $SCRATCH_MNT)
+if [[ "$current_uid" = "Inappropriate ioctl for device while trying to set fs uuid" ]]; then
+ _notrun "UUID ioctls are not supported by kernel."
+fi
+
+# Create filesystem and mount
+_scratch_mkfs_ext4 -O metadata_csum_seed >> $seqres.full 2>&1
+_scratch_mount >> $seqres.full
+
+# Begin fsstress while modifying UUID
+fsstress_args=$(_scale_fsstress_args -d $SCRATCH_MNT -p 15 -n 999999)
+"$FSSTRESS_PROG" $fsstress_args > /dev/null &
+
+test_uuid_ioctl()
+{
+ for n in $(seq 1 20); do
+ new_uuid=$(uuidgen)
+
+ echo "Setting UUID to ${new_uuid}" >> $seqres.full 2>&1
+ $UUID_IOCTL set $SCRATCH_MNT $new_uuid >> $seqres.full 2>&1
+
+ current_uuid=$($UUID_IOCTL get $SCRATCH_MNT)
+ echo "$UUID_IOCTL get $SCARTCH_MNT: $current_uuid" >> $seqres.full 2>&1
+ if [[ "$current_uuid" != "$new_uuid" ]]; then
+ echo "UUID does not equal what was sent with the ioctl"
+ exit
+ fi
+ done
+}
+
+test_uuid_ioctl
+
+# success, all done
+status=0
+exit
diff --git a/tests/ext4/056.out b/tests/ext4/056.out
new file mode 100644
index 00000000..06f52bb4
--- /dev/null
+++ b/tests/ext4/056.out
@@ -0,0 +1 @@
+QA output created by 056
--
2.37.0.rc0.161.g10f37bed90-goog
On Sat, Jun 25, 2022 at 01:53:21AM -0700, Jeremy Bongio wrote:
> Adds a utility to get/set uuid through ext4 ioctl. Executes the ioctls while
> running fsstress. These ioctls are used by tune2fs to safely change the uuid
> without racing other filesystem modifications.
>
> Signed-off-by: Jeremy Bongio <[email protected]>
> ---
Hi Jeremy,
Thanks for the patch, some picky review points as below:
> src/Makefile | 4 +-
> src/global.h | 2 +
> src/uuid_ioctl.c | 106 +++++++++++++++++++++++++++++++++++++++++++++
> tests/ext4/056 | 59 +++++++++++++++++++++++++
> tests/ext4/056.out | 1 +
> 5 files changed, 170 insertions(+), 2 deletions(-)
> create mode 100644 src/uuid_ioctl.c
> create mode 100755 tests/ext4/056
> create mode 100644 tests/ext4/056.out
>
> diff --git a/src/Makefile b/src/Makefile
> index 111ce1d9..e33e04de 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -31,14 +31,14 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \
> fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \
> - detached_mounts_propagation ext4_resize
> + detached_mounts_propagation ext4_resize uuid_ioctl
>
> EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \
> btrfs_crc32c_forged_name.py
>
> SUBDIRS = log-writes perf
>
> -LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt
> +LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt -luuid
>
> ifeq ($(HAVE_XLOG_ASSIGN_LSN), true)
> LINUX_TARGETS += loggen
> diff --git a/src/global.h b/src/global.h
> index b4407099..747d95f8 100644
> --- a/src/global.h
> +++ b/src/global.h
> @@ -184,4 +184,6 @@ roundup_64(unsigned long long x, unsigned int y)
> return rounddown_64(x + y - 1, y);
> }
>
> +#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> +
> #endif /* GLOBAL_H */
> diff --git a/src/uuid_ioctl.c b/src/uuid_ioctl.c
> new file mode 100644
> index 00000000..51480689
> --- /dev/null
> +++ b/src/uuid_ioctl.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Test program which uses the raw ext4 set_fsuuid ioctl directly.
> + * SYNOPSIS:
> + * $0 COMMAND MOUNT_POINT [UUID]
> + *
> + * COMMAND must be either "get" or "set".
> + * The UUID must be a 16 octet represented as 32 hexadecimal digits in canonical
> + * textual representation, e.g. output from `uuidgen`.
> + *
> + */
> +
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <unistd.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <uuid/uuid.h>
> +#include <linux/fs.h>
> +#include "global.h"
> +
> +struct fsuuid {
> + size_t len;
> + __u8 *b;
> +};
> +
> +#ifndef EXT4_IOC_GETFSUUID
> +#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid)
> +#endif
> +
> +#ifndef EXT4_IOC_SETFSUUID
> +#define EXT4_IOC_SETFSUUID _IOW('f', 45, struct fsuuid)
> +#endif
> +
> +int main(int argc, char **argv)
> +{
> + int error, fd;
> + struct fsuuid fsuuid;
^
Bad indent at here, please check the indent of this program.
> +
> + if (argc < 3) {
> + fprintf(stderr, "Invalid arguments\n");
> + return 1;
> + }
> +
> + fd = open(argv[2], O_RDONLY);
> + if (!fd) {
> + perror(argv[2]);
> + return 1;
> + }
> +
> + BUILD_BUG_ON(sizeof(uuid_t) % 16);
> + fsuuid.len = 16;
> + fsuuid.b = calloc(fsuuid.len, sizeof(__u8));
> +
> + if (strcmp(argv[1], "get") == 0) {
> + uuid_t uuid;
> + char uuid_str[37];
> +
> + if (ioctl(fd, EXT4_IOC_GETFSUUID, &fsuuid)) {
> + close(fd);
> + fprintf(stderr, "%s while trying to get fs uuid\n",
> + strerror(errno));
> + return 1;
> + }
> +
> + memcpy(&uuid, fsuuid.b, sizeof(uuid));
> + uuid_unparse(uuid, uuid_str);
> + printf("%s", uuid_str);
> + } else if (strcmp(argv[1], "set") == 0) {
> + if (argc != 4) {
> + fprintf(stderr, "UUID argument missing.\n");
> + return 1;
> + }
> +
> + if (strlen(argv[3]) != 36) {
> + fprintf(stderr, "Invalid UUID. The UUID should be in "
> + "canonical format. Example: "
> + "8c628557-6987-42b2-ba16-b7cc79ddfb43\n");
> + return 1;
> + }
> +
> + uuid_t uuid;
> + error = uuid_parse(argv[3], uuid);
> + if (error < 0) {
> + fprintf(stderr, "%s: invalid UUID.\n", argv[0]);
> + return 1;
> + }
> +
> + memcpy(fsuuid.b, uuid, sizeof(uuid));
> + if(ioctl(fd, EXT4_IOC_SETFSUUID, &fsuuid)) {
> + close(fd);
> + fprintf(stderr, "%s while trying to set fs uuid\n",
> + strerror(errno));
> + return 1;
> + }
> + } else {
> + fprintf(stderr, "Invalid command\n");
> + }
> +
> + close(fd);
> + return 0;
> +}
> diff --git a/tests/ext4/056 b/tests/ext4/056
> new file mode 100755
> index 00000000..46631d46
> --- /dev/null
> +++ b/tests/ext4/056
> @@ -0,0 +1,59 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2000-2005 Silicon Graphics, Inc. All Rights Reserved.
> +#
> +# Test the set/get UUID ioctl.
> +#
> +
> +. ./common/preamble
> +_begin_fstest auto ioctl resize
Why this case is "resize" related?
> +
> +tmpfile="/tmp/$$."
> +trap "rm -f $tmpfile; exit" 0 1 2 3 15
We have default "trap" when you source ./common/preamble. If you need a custom
cleanup by youself, you can define a new one in this case (refer to other cases
which has their own _cleanup).
> +
> +# Import common functions.
> +. ./common/filter
> +
> +# real QA test starts here
> +_supported_fs ext4
> +_require_scratch
> +_require_test_program uuid_ioctl
> +
> +UUID_IOCTL=$here/src/uuid_ioctl
> +
> +# if the ioctl is not supported by the kernel, then skip test.
> +current_uid=$($UUID_IOCTL get $SCRATCH_MNT)
> +if [[ "$current_uid" = "Inappropriate ioctl for device while trying to set fs uuid" ]]; then
> + _notrun "UUID ioctls are not supported by kernel."
^
It's not fault, but we recommended the tab width is 8, for fstests bash script.
> +fi
> +
> +# Create filesystem and mount
> +_scratch_mkfs_ext4 -O metadata_csum_seed >> $seqres.full 2>&1
^
Generally we make sure $seqres.full is null (rm -f $seqres.full) or truncated
at first. So an easier way is using "> $seqres.full" at the first place which
trys to write $seqres.full, to clean .full file.
> +_scratch_mount >> $seqres.full
> +
> +# Begin fsstress while modifying UUID
> +fsstress_args=$(_scale_fsstress_args -d $SCRATCH_MNT -p 15 -n 999999)
> +"$FSSTRESS_PROG" $fsstress_args > /dev/null &
Better to use "2>&1" if you don't want this case fails on some weird fsstress
error output. Or ">>$seqres.full 2>&1" if you'd like to know what happened
on fsstress.
Do I miss something? I didn't find any place try to make sure above fsstress
process is killed/end (by using kill and wait).
> +
> +test_uuid_ioctl()
> +{
> + for n in $(seq 1 20); do
> + new_uuid=$(uuidgen)
I think uuidgen isn't a tool which is 100% installed on all linux system. So
_require_command $UUIDGEN_PROG uuidgen
> +
> + echo "Setting UUID to ${new_uuid}" >> $seqres.full 2>&1
> + $UUID_IOCTL set $SCRATCH_MNT $new_uuid >> $seqres.full 2>&1
> +
> + current_uuid=$($UUID_IOCTL get $SCRATCH_MNT)
> + echo "$UUID_IOCTL get $SCARTCH_MNT: $current_uuid" >> $seqres.full 2>&1
> + if [[ "$current_uuid" != "$new_uuid" ]]; then
> + echo "UUID does not equal what was sent with the ioctl"
> + exit
I think break is enough, but exit is fine too.
> + fi
> + done
> +}
> +
> +test_uuid_ioctl
I'm wondering why not run the code in test_uuid_ioctl directly. Looks like you
just make them into a function, then run this function once. It doesn't use
local variables, or be called many times, or have many complicated lines. Or
you'd like to package the codes in "for n in $(seq 1 20)" loop?
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/ext4/056.out b/tests/ext4/056.out
> new file mode 100644
> index 00000000..06f52bb4
> --- /dev/null
> +++ b/tests/ext4/056.out
> @@ -0,0 +1 @@
> +QA output created by 056
Silence is golden ?
Thanks,
Zorro
> --
> 2.37.0.rc0.161.g10f37bed90-goog
>
On Sun, Jun 26, 2022 at 12:43:56PM +0800, Zorro Lang wrote:
> On Sat, Jun 25, 2022 at 01:53:21AM -0700, Jeremy Bongio wrote:
> > Adds a utility to get/set uuid through ext4 ioctl. Executes the ioctls while
> > running fsstress. These ioctls are used by tune2fs to safely change the uuid
> > without racing other filesystem modifications.
> >
> > Signed-off-by: Jeremy Bongio <[email protected]>
> > ---
>
> Hi Jeremy,
>
> Thanks for the patch, some picky review points as below:
>
> > src/Makefile | 4 +-
> > src/global.h | 2 +
> > src/uuid_ioctl.c | 106 +++++++++++++++++++++++++++++++++++++++++++++
> > tests/ext4/056 | 59 +++++++++++++++++++++++++
> > tests/ext4/056.out | 1 +
> > 5 files changed, 170 insertions(+), 2 deletions(-)
> > create mode 100644 src/uuid_ioctl.c
New programs need a .gitignore entry.
> > create mode 100755 tests/ext4/056
> > create mode 100644 tests/ext4/056.out
> >
> > diff --git a/src/Makefile b/src/Makefile
> > index 111ce1d9..e33e04de 100644
> > --- a/src/Makefile
> > +++ b/src/Makefile
> > @@ -31,14 +31,14 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> > dio-invalidate-cache stat_test t_encrypted_d_revalidate \
> > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \
> > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \
> > - detached_mounts_propagation ext4_resize
> > + detached_mounts_propagation ext4_resize uuid_ioctl
> >
> > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \
> > btrfs_crc32c_forged_name.py
> >
> > SUBDIRS = log-writes perf
> >
> > -LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt
> > +LLDLIBS = $(LIBHANDLE) $(LIBACL) -lpthread -lrt -luuid
> >
> > ifeq ($(HAVE_XLOG_ASSIGN_LSN), true)
> > LINUX_TARGETS += loggen
> > diff --git a/src/global.h b/src/global.h
> > index b4407099..747d95f8 100644
> > --- a/src/global.h
> > +++ b/src/global.h
> > @@ -184,4 +184,6 @@ roundup_64(unsigned long long x, unsigned int y)
> > return rounddown_64(x + y - 1, y);
> > }
> >
> > +#define BUILD_BUG_ON(condition) ((void)sizeof(char[1 - 2*!!(condition)]))
> > +
> > #endif /* GLOBAL_H */
> > diff --git a/src/uuid_ioctl.c b/src/uuid_ioctl.c
> > new file mode 100644
> > index 00000000..51480689
> > --- /dev/null
> > +++ b/src/uuid_ioctl.c
> > @@ -0,0 +1,106 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Test program which uses the raw ext4 set_fsuuid ioctl directly.
> > + * SYNOPSIS:
> > + * $0 COMMAND MOUNT_POINT [UUID]
> > + *
> > + * COMMAND must be either "get" or "set".
> > + * The UUID must be a 16 octet represented as 32 hexadecimal digits in canonical
> > + * textual representation, e.g. output from `uuidgen`.
> > + *
> > + */
> > +
> > +#include <stdio.h>
> > +#include <fcntl.h>
> > +#include <errno.h>
> > +#include <unistd.h>
> > +#include <stdint.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <uuid/uuid.h>
> > +#include <linux/fs.h>
> > +#include "global.h"
> > +
> > +struct fsuuid {
> > + size_t len;
> > + __u8 *b;
> > +};
> > +
> > +#ifndef EXT4_IOC_GETFSUUID
> > +#define EXT4_IOC_GETFSUUID _IOR('f', 44, struct fsuuid)
> > +#endif
> > +
> > +#ifndef EXT4_IOC_SETFSUUID
> > +#define EXT4_IOC_SETFSUUID _IOW('f', 45, struct fsuuid)
> > +#endif
> > +
> > +int main(int argc, char **argv)
> > +{
> > + int error, fd;
> > + struct fsuuid fsuuid;
> ^
>
> Bad indent at here, please check the indent of this program.
>
> > +
> > + if (argc < 3) {
> > + fprintf(stderr, "Invalid arguments\n");
> > + return 1;
> > + }
> > +
> > + fd = open(argv[2], O_RDONLY);
> > + if (!fd) {
> > + perror(argv[2]);
> > + return 1;
> > + }
> > +
> > + BUILD_BUG_ON(sizeof(uuid_t) % 16);
I'm not sure why we care that the size is a multiple of 16?
> > + fsuuid.len = 16;
fsuuid.len = sizeof(uuid_t) ?
> > + fsuuid.b = calloc(fsuuid.len, sizeof(__u8));
> > +
> > + if (strcmp(argv[1], "get") == 0) {
> > + uuid_t uuid;
> > + char uuid_str[37];
> > +
> > + if (ioctl(fd, EXT4_IOC_GETFSUUID, &fsuuid)) {
> > + close(fd);
> > + fprintf(stderr, "%s while trying to get fs uuid\n",
> > + strerror(errno));
If the close() fails it will obliterate the errno set by the ioctl.
if (ioctl(...) == -1) {
perror("GETFSUUID");
close(fd);
return 1;
}
(Also, this is a test program; if you're merely going to exit, then
don't bother closing the fds.)
> > + return 1;
> > + }
> > +
> > + memcpy(&uuid, fsuuid.b, sizeof(uuid));
> > + uuid_unparse(uuid, uuid_str);
> > + printf("%s", uuid_str);
> > + } else if (strcmp(argv[1], "set") == 0) {
> > + if (argc != 4) {
> > + fprintf(stderr, "UUID argument missing.\n");
> > + return 1;
> > + }
> > +
> > + if (strlen(argv[3]) != 36) {
> > + fprintf(stderr, "Invalid UUID. The UUID should be in "
> > + "canonical format. Example: "
> > + "8c628557-6987-42b2-ba16-b7cc79ddfb43\n");
Won't uuid_parse catch an overly long string?
> > + return 1;
> > + }
> > +
> > + uuid_t uuid;
> > + error = uuid_parse(argv[3], uuid);
> > + if (error < 0) {
> > + fprintf(stderr, "%s: invalid UUID.\n", argv[0]);
> > + return 1;
> > + }
> > +
> > + memcpy(fsuuid.b, uuid, sizeof(uuid));
> > + if(ioctl(fd, EXT4_IOC_SETFSUUID, &fsuuid)) {
Space before the ^ left paren.
> > + close(fd);
> > + fprintf(stderr, "%s while trying to set fs uuid\n",
> > + strerror(errno));
> > + return 1;
> > + }
> > + } else {
> > + fprintf(stderr, "Invalid command\n");
No return 1 here?
> > + }
> > +
> > + close(fd);
> > + return 0;
> > +}
> > diff --git a/tests/ext4/056 b/tests/ext4/056
> > new file mode 100755
> > index 00000000..46631d46
> > --- /dev/null
> > +++ b/tests/ext4/056
> > @@ -0,0 +1,59 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2000-2005 Silicon Graphics, Inc. All Rights Reserved.
You work for SGI???
> > +#
> > +# Test the set/get UUID ioctl.
> > +#
> > +
> > +. ./common/preamble
> > +_begin_fstest auto ioctl resize
>
> Why this case is "resize" related?
>
> > +
> > +tmpfile="/tmp/$$."
> > +trap "rm -f $tmpfile; exit" 0 1 2 3 15
>
> We have default "trap" when you source ./common/preamble. If you need a custom
> cleanup by youself, you can define a new one in this case (refer to other cases
> which has their own _cleanup).
>
> > +
> > +# Import common functions.
> > +. ./common/filter
> > +
> > +# real QA test starts here
> > +_supported_fs ext4
> > +_require_scratch
> > +_require_test_program uuid_ioctl
> > +
> > +UUID_IOCTL=$here/src/uuid_ioctl
> > +
> > +# if the ioctl is not supported by the kernel, then skip test.
> > +current_uid=$($UUID_IOCTL get $SCRATCH_MNT)
> > +if [[ "$current_uid" = "Inappropriate ioctl for device while trying to set fs uuid" ]]; then
While trying to **set** fs uuid? The command above retrieves it, right?
Also, didn't the test program print that error message to stderr? Which
the $() backticks don't capture.
> > + _notrun "UUID ioctls are not supported by kernel."
> ^
>
> It's not fault, but we recommended the tab width is 8, for fstests bash script.
>
> > +fi
> > +
> > +# Create filesystem and mount
> > +_scratch_mkfs_ext4 -O metadata_csum_seed >> $seqres.full 2>&1
> ^
> Generally we make sure $seqres.full is null (rm -f $seqres.full) or truncated
> at first. So an easier way is using "> $seqres.full" at the first place which
> trys to write $seqres.full, to clean .full file.
The preamble always does this, so the append is fine.
> > +_scratch_mount >> $seqres.full
> > +
> > +# Begin fsstress while modifying UUID
> > +fsstress_args=$(_scale_fsstress_args -d $SCRATCH_MNT -p 15 -n 999999)
> > +"$FSSTRESS_PROG" $fsstress_args > /dev/null &
>
> Better to use "2>&1" if you don't want this case fails on some weird fsstress
> error output. Or ">>$seqres.full 2>&1" if you'd like to know what happened
> on fsstress.
>
> Do I miss something? I didn't find any place try to make sure above fsstress
> process is killed/end (by using kill and wait).
Right -- the test should kill fsstress once it's done.
> > +
> > +test_uuid_ioctl()
> > +{
> > + for n in $(seq 1 20); do
> > + new_uuid=$(uuidgen)
>
> I think uuidgen isn't a tool which is 100% installed on all linux system. So
>
> _require_command $UUIDGEN_PROG uuidgen
>
> > +
> > + echo "Setting UUID to ${new_uuid}" >> $seqres.full 2>&1
> > + $UUID_IOCTL set $SCRATCH_MNT $new_uuid >> $seqres.full 2>&1
Shouldn't unexpected errors be recorded in the golden output?
> > +
> > + current_uuid=$($UUID_IOCTL get $SCRATCH_MNT)
> > + echo "$UUID_IOCTL get $SCARTCH_MNT: $current_uuid" >> $seqres.full 2>&1
> > + if [[ "$current_uuid" != "$new_uuid" ]]; then
> > + echo "UUID does not equal what was sent with the ioctl"
> > + exit
>
> I think break is enough, but exit is fine too.
Why even break? Let the test continue running and recording all the
failures it encounters, not just the first one.
--D
>
> > + fi
> > + done
> > +}
> > +
> > +test_uuid_ioctl
>
> I'm wondering why not run the code in test_uuid_ioctl directly. Looks like you
> just make them into a function, then run this function once. It doesn't use
> local variables, or be called many times, or have many complicated lines. Or
> you'd like to package the codes in "for n in $(seq 1 20)" loop?
>
> > +
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/ext4/056.out b/tests/ext4/056.out
> > new file mode 100644
> > index 00000000..06f52bb4
> > --- /dev/null
> > +++ b/tests/ext4/056.out
> > @@ -0,0 +1 @@
> > +QA output created by 056
>
> Silence is golden ?
>
> Thanks,
> Zorro
>
> > --
> > 2.37.0.rc0.161.g10f37bed90-goog
> >
>