Hi Willow, Nick,
I am not convinced that the openat()/xopenat() refactor is necessary for this
SRU.
It renames many functions over many files, with quite a considerable diff, for
no real benefit.
The only usage of the new style functions are two calls to openat() in the test
method present in lp2088163-copy-introduce-COPY_VERIFY_LINKED-flag.patch.
+ fd_1 = openat(tfd, "hoge", O_CLOEXEC | O_NOCTTY | O_RDONLY);
+ fd_2 = openat(tfd, "hoge", O_CLOEXEC | O_NOCTTY | O_RDONLY);
as well as the calls to copy_file_at():
+ assert_se(copy_file_at(fd_1, NULL, tfd, "to_1", 0, 0644, 0) >= 0);
...
+ assert_se(copy_file_at(fd_2, NULL, tfd, "to_2", O_EXCL, 0644,
COPY_VERIFY_LINKED) == -EIDRM);
I looked into it, and the calls to openat() can stay the exact same. We only
need to adjust copy_file_at(): The diff would be:
- assert_se(copy_file_at(fd_1, NULL, tfd, "to_1", 0, 0644, 0) >= 0);
+ assert_se(copy_file_at(AT_FDCWD, FORMAT_PROC_FD_PATH(fd_1), tfd,
"to_1", 0, 0644, 0) >= 0);
- assert_se(copy_file_at(fd_2, NULL, tfd, "to_2", O_EXCL, 0644,
COPY_VERIFY_LINKED) == -EIDRM);
+ assert_se(copy_file_at(AT_FDCWD, FORMAT_PROC_FD_PATH(fd_2), tfd,
"to_2", O_EXCL, 0644, COPY_VERIFY_LINKED) == -EIDRM);
with just this diff, we can drop the entirety of
lp2088163-fs-util-rename-xopenat-xopanat_full.patch
lp2088163-copy-use-xopenat-to-make-from-argument-optional.patch
which is quite substantial.
Additionally, I am not convinced that
lp2088163-journal-file-util-use-the-file-descriptor-of-journal-
file.patch
is required either. Surely we could just make a slight backport change
in
lp2088163-journal-file-util-use-COPY_VERIFY_LINKED.patch
to resemble:
--- a/src/shared/journal-file-util.c
+++ b/src/shared/journal-file-util.c
@@ -210,10 +210,14 @@ static void journal_file_set_offline_int
log_debug_errno(r, "Failed to re-enable
copy-on-write for %s: %m, rewriting file", f->path);
+ /* Here, setting COPY_VERIFY_LINKED flag is
crucial. Otherwise, a broken
+ * journal file may be created, if
journal_directory_vacuum() ->
+ * unlinkat_deallocate() is called in the main
thread while this thread is
+ * copying the file. See issue #24150 and
#31222. */
r =
copy_file_atomic_full(FORMAT_PROC_FD_PATH(f->fd), f->path, f->mode,
0,
FS_NOCOW_FL,
- COPY_REPLACE |
COPY_FSYNC | COPY_HOLES | COPY_ALL_XATTRS,
+ COPY_REPLACE |
COPY_FSYNC | COPY_HOLES | COPY_ALL_XATTRS | COPY_VERIFY_LINKED,
NULL, NULL);
if (r < 0) {
log_debug_errno(r, "Failed to rewrite
%s: %m", f->path);
instead? Then we can also drop
lp2088163-journal-file-util-use-the-file-descriptor-of-journal-file.patch
completely.
I know these three patches are present in the systemd v255 stable backport tree,
but they add quite considerable size to the SRU for very little gain.
lp2088163-fs-util-rename-xopenat-xopanat_full.patch
lp2088163-copy-use-xopenat-to-make-from-argument-optional.patch
lp2088163-journal-file-util-use-the-file-descriptor-of-journal-file.patch
I think the rest of the patches are okay.
lp2088163-stat-util-introduce-stat-fd-_verify_linked.patch
lp2088163-sd-journal-use-stat_verify_linked.patch
lp2088163-copy-introduce-COPY_VERIFY_LINKED-flag.patch
lp2088163-journal-file-util-use-COPY_VERIFY_LINKED.patch
lp2088163-test-add-test-cases-for-journal-corruption-on-btrfs.patch
What do you think?
Thanks,
Matthew
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/2088163
Title:
Journald file corruption
To manage notifications about this bug go to:
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/2088163/+subscriptions
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
