| Message ID | 20210116165619.494265-4-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tests for overlayfs immutable/append-only files | expand |
On Sat, Jan 16, 2021 at 06:56:18PM +0200, Amir Goldstein wrote: > For overlayfs tests we need to be able to setflags on existing > (lower) files. > > t_immutable -C test_dir > > Creates the test area and sets flags, but it also allows setting flags > on an existing test area. > > t_immutable -R test_dir > > Removes the flags from existing test area, but does not remove the files > in the test area. > > To setup a test area with file without flags, need to run the -C and -R > commands. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > src/t_immutable.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/src/t_immutable.c b/src/t_immutable.c > index b6a76af0..a2e6796d 100644 > --- a/src/t_immutable.c > +++ b/src/t_immutable.c > @@ -1898,6 +1898,8 @@ static int check_test_area(const char *dir) > return 0; > } > > +static int allow_existing; > + > static int create_dir(char **ppath, const char *fmt, const char *dir) > { > const char *path; > @@ -1908,6 +1910,9 @@ static int create_dir(char **ppath, const char *fmt, const char *dir) > } > path = *ppath; > if (stat(path, &st) == 0) { > + if (allow_existing && S_ISDIR(st.st_mode)) { > + return 0; > + } > fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n", > __progname, path); > return 1; > @@ -1921,6 +1926,7 @@ static int create_dir(char **ppath, const char *fmt, const char *dir) > > static int create_file(char **ppath, const char *fmt, const char *dir) > { > + int flags = O_WRONLY|O_CREAT | (allow_existing ? 0 : O_EXCL); > const char *path; > int fd; > > @@ -1928,7 +1934,7 @@ static int create_file(char **ppath, const char *fmt, const char *dir) > return -1; > } > path = *ppath; > - if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) { > + if ((fd = open(path, flags, 0666)) == -1) { > fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno)); > return -1; > } > @@ -1937,13 +1943,15 @@ static int create_file(char **ppath, const char *fmt, const char *dir) > > static int create_xattrs(int fd) > { > - if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) { > + int flags = allow_existing ? 0 : XATTR_CREATE; > + > + if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), flags) != 0) { > if (errno != EOPNOTSUPP) { > perror("setxattr"); > return 1; > } > } > - if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) { > + if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), flags) != 0) { > if (errno != EOPNOTSUPP) { > perror("setxattr"); > return 1; > @@ -2214,6 +2222,10 @@ static int remove_test_area(const char *dir) > return 1; > } > > + if (allow_existing) { > + return 0; > + } > + > pid = fork(); > if (!pid) { > execl("/bin/rm", "rm", "-rf", dir, NULL); > @@ -2236,7 +2248,7 @@ int main(int argc, char **argv) > /* this arg parsing is gross, but who cares, its a test program */ > > if (argc < 2) { > - fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n"); > + fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n"); > return 1; > } > > @@ -2246,18 +2258,24 @@ int main(int argc, char **argv) > /* Prepare test area without running tests */ > create = 1; > runtest = 0; > + /* With existing test area, only setflags */ > + allow_existing = 1; > } else if (!strcmp(argv[1], "-r")) { > remove = 1; > + } else if (!strcmp(argv[1], "-R")) { > + /* Cleanup flags on test area but leave the files */ > + remove = 1; > + allow_existing = 1; > } > > if (argc != 2 + (create | remove)) { > - fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n"); > + fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n"); > return 1; > } > > if (create) { > ret = create_test_area(argv[argc-1]); > - if (ret || !runtest) { > + if (ret || allow_existing) { With this change, compiler warns about 'runtest' is set but not used, and 'allow_existing' now indicates '!runtest' implicitly, which seems subtle. I think it's better to keep 'runtest' as the indicator to actually run the test? Thanks, Eryu > return ret; > } > } else if (remove) { > -- > 2.25.1
On Sun, Jan 24, 2021 at 5:14 PM Eryu Guan <guan@eryu.me> wrote: > > On Sat, Jan 16, 2021 at 06:56:18PM +0200, Amir Goldstein wrote: > > For overlayfs tests we need to be able to setflags on existing > > (lower) files. > > > > t_immutable -C test_dir > > > > Creates the test area and sets flags, but it also allows setting flags > > on an existing test area. > > > > t_immutable -R test_dir > > > > Removes the flags from existing test area, but does not remove the files > > in the test area. > > > > To setup a test area with file without flags, need to run the -C and -R > > commands. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > src/t_immutable.c | 30 ++++++++++++++++++++++++------ > > 1 file changed, 24 insertions(+), 6 deletions(-) > > > > diff --git a/src/t_immutable.c b/src/t_immutable.c > > index b6a76af0..a2e6796d 100644 > > --- a/src/t_immutable.c > > +++ b/src/t_immutable.c > > @@ -1898,6 +1898,8 @@ static int check_test_area(const char *dir) > > return 0; > > } > > > > +static int allow_existing; > > + > > static int create_dir(char **ppath, const char *fmt, const char *dir) > > { > > const char *path; > > @@ -1908,6 +1910,9 @@ static int create_dir(char **ppath, const char *fmt, const char *dir) > > } > > path = *ppath; > > if (stat(path, &st) == 0) { > > + if (allow_existing && S_ISDIR(st.st_mode)) { > > + return 0; > > + } > > fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n", > > __progname, path); > > return 1; > > @@ -1921,6 +1926,7 @@ static int create_dir(char **ppath, const char *fmt, const char *dir) > > > > static int create_file(char **ppath, const char *fmt, const char *dir) > > { > > + int flags = O_WRONLY|O_CREAT | (allow_existing ? 0 : O_EXCL); > > const char *path; > > int fd; > > > > @@ -1928,7 +1934,7 @@ static int create_file(char **ppath, const char *fmt, const char *dir) > > return -1; > > } > > path = *ppath; > > - if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) { > > + if ((fd = open(path, flags, 0666)) == -1) { > > fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno)); > > return -1; > > } > > @@ -1937,13 +1943,15 @@ static int create_file(char **ppath, const char *fmt, const char *dir) > > > > static int create_xattrs(int fd) > > { > > - if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) { > > + int flags = allow_existing ? 0 : XATTR_CREATE; > > + > > + if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), flags) != 0) { > > if (errno != EOPNOTSUPP) { > > perror("setxattr"); > > return 1; > > } > > } > > - if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) { > > + if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), flags) != 0) { > > if (errno != EOPNOTSUPP) { > > perror("setxattr"); > > return 1; > > @@ -2214,6 +2222,10 @@ static int remove_test_area(const char *dir) > > return 1; > > } > > > > + if (allow_existing) { > > + return 0; > > + } > > + > > pid = fork(); > > if (!pid) { > > execl("/bin/rm", "rm", "-rf", dir, NULL); > > @@ -2236,7 +2248,7 @@ int main(int argc, char **argv) > > /* this arg parsing is gross, but who cares, its a test program */ > > > > if (argc < 2) { > > - fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n"); > > + fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n"); > > return 1; > > } > > > > @@ -2246,18 +2258,24 @@ int main(int argc, char **argv) > > /* Prepare test area without running tests */ > > create = 1; > > runtest = 0; > > + /* With existing test area, only setflags */ > > + allow_existing = 1; > > } else if (!strcmp(argv[1], "-r")) { > > remove = 1; > > + } else if (!strcmp(argv[1], "-R")) { > > + /* Cleanup flags on test area but leave the files */ > > + remove = 1; > > + allow_existing = 1; > > } > > > > if (argc != 2 + (create | remove)) { > > - fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n"); > > + fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n"); > > return 1; > > } > > > > if (create) { > > ret = create_test_area(argv[argc-1]); > > - if (ret || !runtest) { > > + if (ret || allow_existing) { > > With this change, compiler warns about 'runtest' is set but not used, > and 'allow_existing' now indicates '!runtest' implicitly, which seems > subtle. I think it's better to keep 'runtest' as the indicator to > actually run the test? > Sure, I removed it by mistake. Thanks, Amir.
On Sun, Jan 24, 2021 at 05:32:15PM +0200, Amir Goldstein wrote: > On Sun, Jan 24, 2021 at 5:14 PM Eryu Guan <guan@eryu.me> wrote: > > [snap] > > > > > > if (create) { > > > ret = create_test_area(argv[argc-1]); > > > - if (ret || !runtest) { > > > + if (ret || allow_existing) { > > > > With this change, compiler warns about 'runtest' is set but not used, > > and 'allow_existing' now indicates '!runtest' implicitly, which seems > > subtle. I think it's better to keep 'runtest' as the indicator to > > actually run the test? > > > > Sure, I removed it by mistake. Then this is the only place that needs update. I can fix it on commit, no need to resend then. Thanks, Eryu
On Mon, Jan 25, 2021 at 2:46 PM Eryu Guan <eguan@linux.alibaba.com> wrote: > > On Sun, Jan 24, 2021 at 05:32:15PM +0200, Amir Goldstein wrote: > > On Sun, Jan 24, 2021 at 5:14 PM Eryu Guan <guan@eryu.me> wrote: > > > > [snap] > > > > > > > > if (create) { > > > > ret = create_test_area(argv[argc-1]); > > > > - if (ret || !runtest) { > > > > + if (ret || allow_existing) { > > > > > > With this change, compiler warns about 'runtest' is set but not used, > > > and 'allow_existing' now indicates '!runtest' implicitly, which seems > > > subtle. I think it's better to keep 'runtest' as the indicator to > > > actually run the test? > > > > > > > Sure, I removed it by mistake. > > Then this is the only place that needs update. I can fix it on commit, > no need to resend then. > Excellent. Now, about that kernel deadlock mentioned in is commented out line in test overlay/075. The fix for that is in overlayfs-next: 147ec02b8705 - ovl: avoid deadlock on directory ioctl But I am not so happy about adding a test that crashes stable/old kernels. If you like I can post another test that is "dangerous" just for the deadlock but after the fix is merged to master and 5.10.y so at least people who tests the latest kernels will not crash. Let me know your preference. Thanks, Amir.
diff --git a/src/t_immutable.c b/src/t_immutable.c index b6a76af0..a2e6796d 100644 --- a/src/t_immutable.c +++ b/src/t_immutable.c @@ -1898,6 +1898,8 @@ static int check_test_area(const char *dir) return 0; } +static int allow_existing; + static int create_dir(char **ppath, const char *fmt, const char *dir) { const char *path; @@ -1908,6 +1910,9 @@ static int create_dir(char **ppath, const char *fmt, const char *dir) } path = *ppath; if (stat(path, &st) == 0) { + if (allow_existing && S_ISDIR(st.st_mode)) { + return 0; + } fprintf(stderr, "%s: Test area directory %s must not exist for test area creation.\n", __progname, path); return 1; @@ -1921,6 +1926,7 @@ static int create_dir(char **ppath, const char *fmt, const char *dir) static int create_file(char **ppath, const char *fmt, const char *dir) { + int flags = O_WRONLY|O_CREAT | (allow_existing ? 0 : O_EXCL); const char *path; int fd; @@ -1928,7 +1934,7 @@ static int create_file(char **ppath, const char *fmt, const char *dir) return -1; } path = *ppath; - if ((fd = open(path, O_WRONLY|O_CREAT|O_EXCL, 0666)) == -1) { + if ((fd = open(path, flags, 0666)) == -1) { fprintf(stderr, "%s: error creating file %s: %s\n", __progname, path, strerror(errno)); return -1; } @@ -1937,13 +1943,15 @@ static int create_file(char **ppath, const char *fmt, const char *dir) static int create_xattrs(int fd) { - if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) { + int flags = allow_existing ? 0 : XATTR_CREATE; + + if (fsetxattr(fd, "trusted.test", "readonly", strlen("readonly"), flags) != 0) { if (errno != EOPNOTSUPP) { perror("setxattr"); return 1; } } - if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), XATTR_CREATE) != 0) { + if (fsetxattr(fd, "user.test", "readonly", strlen("readonly"), flags) != 0) { if (errno != EOPNOTSUPP) { perror("setxattr"); return 1; @@ -2214,6 +2222,10 @@ static int remove_test_area(const char *dir) return 1; } + if (allow_existing) { + return 0; + } + pid = fork(); if (!pid) { execl("/bin/rm", "rm", "-rf", dir, NULL); @@ -2236,7 +2248,7 @@ int main(int argc, char **argv) /* this arg parsing is gross, but who cares, its a test program */ if (argc < 2) { - fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n"); + fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n"); return 1; } @@ -2246,18 +2258,24 @@ int main(int argc, char **argv) /* Prepare test area without running tests */ create = 1; runtest = 0; + /* With existing test area, only setflags */ + allow_existing = 1; } else if (!strcmp(argv[1], "-r")) { remove = 1; + } else if (!strcmp(argv[1], "-R")) { + /* Cleanup flags on test area but leave the files */ + remove = 1; + allow_existing = 1; } if (argc != 2 + (create | remove)) { - fprintf(stderr, "usage: t_immutable [-C|-c|-r] test_area_dir\n"); + fprintf(stderr, "usage: t_immutable [-C|-c|-R|-r] test_area_dir\n"); return 1; } if (create) { ret = create_test_area(argv[argc-1]); - if (ret || !runtest) { + if (ret || allow_existing) { return ret; } } else if (remove) {
For overlayfs tests we need to be able to setflags on existing (lower) files. t_immutable -C test_dir Creates the test area and sets flags, but it also allows setting flags on an existing test area. t_immutable -R test_dir Removes the flags from existing test area, but does not remove the files in the test area. To setup a test area with file without flags, need to run the -C and -R commands. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- src/t_immutable.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)