Remove stray newline in filesystem show #866
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The newlines separate listings of multiple filesystems, with this patch it's all in one long text and not easy to visually skim just one filesystem. Showing a single filesystem does have a stray newline so this can be fixed but I'd rather keep the visual style of multiple filesystems. |
|
Makes sense to me! Do you think a refactor here to drop the newline on the last filesystem would be reasonable? |
kdave
force-pushed
the
devel
branch
2 times, most recently
from
8063721 to
4636c58
Compare
|
Yes, this looks good. There are some minor issues, I'll add the review comments. |
kdave
requested changes
Aug 14, 2024
Fixes so 'python3 -m build' works and package can be uploaded to pypi (https://pypi.org/project/btrfsutil/). - setup.py is still used for local build (make) - for pypi it must be done by 'python3 -m build' that is build in a temporary directory - btrfsutilpy.h must be also distributed - version is set manually (the git VERSION file is not accessible) - the project page metadata is empty, the README.md should be added Issue: kdave#310 Signed-off-by: David Sterba <[email protected]>
Last minute documentation changes before release that did not go through CI. Signed-off-by: David Sterba <[email protected]>
[ci skip] Signed-off-by: David Sterba <[email protected]>
All issues have been fixed in latest master, enable the checks for devel too. It takes about 17m. Also rename the file, drop the "ci-" prefix. Signed-off-by: David Sterba <[email protected]>
We use the UASSERT() wrapper instead of the plain assert() as this can be tuned to print the stack trace too if supported. Signed-off-by: David Sterba <[email protected]>
The name is never touched, thus it should be const. Reviewed-by: Boris Burkov <[email protected]> Signed-off-by: Qu Wenruo <[email protected]>
[PITFALLS] There are several hidden pitfalls of the existing traverse_directory(): - Hand written preorder traversal There is already a better written standard library function, nftw() doing exactly what we need. - Over-designed path list To properly handle the directory change, we have structure directory_name_entry, to record every inode until rootdir. But it has two string members, dir_name and path, which is a little confusing and overkilled. As for preorder traversal, we will never need to read the parent's filename, just its btrfs inode number. And it's exported while no one utilizes it out of mkfs/rootdir.c. - Weird inode numbers We use the inode number from st->st_ino, with an extra offset. This by itself is not safe, if the rootdir has child directories in another filesystem. And this results very weird inode numbers, e.g: item 0 key (256 INODE_ITEM 0) itemoff 16123 itemsize 160 item 6 key (88347519 INODE_ITEM 0) itemoff 15815 itemsize 160 item 16 key (88347520 INODE_ITEM 0) itemoff 15363 itemsize 160 item 20 key (88347521 INODE_ITEM 0) itemoff 15119 itemsize 160 item 24 key (88347522 INODE_ITEM 0) itemoff 14875 itemsize 160 item 26 key (88347523 INODE_ITEM 0) itemoff 14700 itemsize 160 item 28 key (88347524 INODE_ITEM 0) itemoff 14525 itemsize 160 item 30 key (88347557 INODE_ITEM 0) itemoff 14350 itemsize 160 item 32 key (88347566 INODE_ITEM 0) itemoff 14175 itemsize 160 Which is far from a regular fs created by copying the data. - Weird directory inode size calculation Unlike kernel, which updated the directory inode size every time new child inodes are added, we calculate the directory inode size by searching all its children first, then later new inodes linked to this directory won't touch the inode size. - Bad hard link detection and cross mount point handling The hard link detection is purely based on the st_ino returned from the host filesystem, this means we do not have extra checks whether the inode is even inside the same fs. And we directly reuse st_nlink from the host filesystem, if there is a hard link out of rootdir, the st_nlink will be incorrect and cause a corrupted fs. Enhance all these points by: - Use nftw() to do the preorder traversal It also provides the extra level detection, which is pretty handy. - Use a simple local inode_entry to record each parent The only value is a u64 to record the inode number. And one simple rootdir_path structure to record the list of inode_entry, alone with the current level. This rootdir_path structure along with two helpers, rootdir_path_push() and rootdir_path_pop(), along with the preorder traversal provided by nftw(), are enough for us to record all the parent directories until the rootdir. - Grab new inode number properly Just call btrfs_get_free_objectid() to grab a proper inode number, other than using some weird calculated value. - Treat every inode as a new one This means we will have no hard link support for now. But I still believe it's a good trade-off, especially considering the old handling is buggy for several corner cases. - Use btrfs_insert_inode() and btrfs_add_link() to update directory inode automatically With all the refactoring, the code is shorter and easier to read. Reviewed-by: Boris Burkov <[email protected]> Signed-off-by: Qu Wenruo <[email protected]>
The recent rework changes how we detect hard links. [OLD BEHAVIOR] We trusted st_nlink and st_ino, reuse them without extra sanity checks. That behavior has problems handling cross mount-point or hard links out of the rootdir cases. [NEW BEHAVIOR] The new refactored code will treat every inode, no matter if it's a hardlink, as a new inode. This means we will break the hard link detection, and every hard link will be created as a different inode. For the most common use case, like populating a rootfs, it's totally fine. [EXTRA WARNING] But for cases where the user have extra hard links inside the rootdir, output a warning just to inform the end user. This will not cause any content difference, just breaking the hard links into new inodes. Signed-off-by: Qu Wenruo <[email protected]>
The test case will create the following directory layout: . |- rootdir/ | |- inside_link |- outside_link Both inside_link and outside_link are hard links of each other. And use rootdir/ as the rootdir for mkfs. This is to ensure the nlink of inside_link is correctly set to 1. Inspired by the recent rework which fixes the handling of hard links. Signed-off-by: Qu Wenruo <[email protected]>
The new test case creates a special layout like this: rootdir/ (fs1 ino=256) |- dir1/ (fs1 ino=257) | |- dir1/ (fs2 ino=257) | |- dir2/ (fs2 ino=258) | |- file1 (fs2 ino=259) | |- file2 (fs2 ino=260) |- dir2/ (fs1 ino=258) |- file1 (fs1 ino=259) |- file2 (fs2 ino=259) This layout intentionally creates inode number conflicts, which will make the old "mkfs.btrfs --rootdir" to fail. But newer reworked one will successfully handle them, just leave a test case to avoid to hit the old bugs. Signed-off-by: Qu Wenruo <[email protected]>
Commit 14ac1a6 ("btrfs-progs: mkfs: add support for squota") mistakenly added ctree.h from libbtrfs/ but this is not supposed to be used outside of the library. Moreover the correct ctree.h was already there. Signed-off-by: David Sterba <[email protected]>
There are many definitions of types etc that are not used in libbtrfs but used to be in other code before the separation. Remove it as it's not meant to be exported. Signed-off-by: David Sterba <[email protected]>
Remove BUG_ONs that seem to be sanity checks that are done in other places. We want to remove them from public header so BUG_ON can be removed completely. Signed-off-by: David Sterba <[email protected]>
Commit bf0f3db ("btrfs-progs: introduce UASSERT() for purely userspace code") added UASERT to distinguish ASSERT macro from user space code. This was wrongly added to libbtrfs/ and pulled the common/messages.h include too. Signed-off-by: David Sterba <[email protected]>
The stack trace and BUG_ON related reporting was inherited from the tools, this should not be part of libbtrfs and was not intended to be exported. BUG() is still in used in ctree.h and send-utils.c so replace it with a bare error report and remove the rest. Keep __always_inline as it's needed for Musl. Signed-off-by: David Sterba <[email protected]>
The BUILD_ASSERT macro checks what _Static_assert can do. Remove it as it's not really used in ioctl.h as it defines a stub. The assertions still remain in the code outside of libbtrfs, we can delete it here as the API is frozen and won't be changed. Signed-off-by: David Sterba <[email protected]>
Signed-off-by: David Sterba <[email protected]>
The list_head is used in struct definitions but otherwise not at all as it was copied from kernel code. For ctree.h add stub definition that won't change the containing structure size. Drop list.h from libbtrfs. This may break some builds if they used the header, though this was never meant to be exported. Signed-off-by: David Sterba <[email protected]>
None of the public API uses the rb-tree code besides definitions, so change the includes in ctree.h and drop rbtree.h, this is used only by internal implementation in send-utils.c. We could remove it in the future but last time it was not possible due to 3rd party code depending on it. Removed in 83ab925 ("libbtrfs: remove the support for fs without uuid tree") and reverted again in f9b0da8 ("Revert "libbtrfs: remove the support for fs without uuid tree"") Signed-off-by: David Sterba <[email protected]>
There are two places defining the checker stub macros, merge them to one place. Signed-off-by: David Sterba <[email protected]>
There are no functional changes, only cleanup of header files. This could lead to build failures in case the headers were used as a convenience outside of scope of libbtrfs just because of the kernel compatibility. - removed various definitions of variables, types, helpers and macros from kerncompat.h that are neither used nor needed for libbtrfs code - file list.h no longer shipped - file rbtree.h no longer shipped Signed-off-by: David Sterba <[email protected]>
__set_bit and __clear_bit are unused and redundant, we have them in kernel-lib/bitops.h as well. Signed-off-by: David Sterba <[email protected]>
Sync up with kernel and fix warnings reported by -Wcast-qual. eg. Most of the change is due to extent_buffer::data, which is a direct struct member, unlike in kernel where it's an array of pages. The const qualifier cannot be used the same way so it's dropped in affected herlpers. Signed-off-by: David Sterba <[email protected]>
Copy linux.git/include/linux/container_of.h definition of container_of and the const variant (currently unused) Signed-off-by: David Sterba <[email protected]>
…in reverse direction process_clone() only searches the received_uuid, but could exist in an earlier uuid that isn't the received_uuid. Mirror what process_snapshot does and search both the received_uuid and if that fails look up by normal uuid. Fixes: kdave#606 Issue: kdave#606 Pull-request: kdave#643 Pull-request: kdave#862 Signed-off-by: Arsenii Skvortsov <[email protected]> Signed-off-by: David Sterba <[email protected]>
The way the CRC32C checksum used for btrfs-send differs from the way it's used elsewhere in btrfs. Without making the distinction, it's easy to make the flawed assumption that CRC32C always refers to the same, and end up with code that produces the wrong checksums. This small note should guide the reader to the right function. The best notes on the protocol I found are here: https://archive.kernel.org/oldwiki/btrfs.wiki.kernel.org/index.php/Design_notes_on_Send/Receive.html The crc32c might be used in two meanings and this could be confusing when implementing the send stream protocol. Rust code describing the algorithm for the crc crate that worked for me: pub const CRC_32_BTRFS_SEND: crc::Algorithm<u32> = crc::Algorithm { width: 32, poly: 0x1edc6f41, init: 0, refin: true, refout: true, xorout: 0, check: 0xe3069283, residue: 0xb798b438 }; (it's a slight variation on the one used in ISCSI) Note: Documentation/dev/dev-send-stream.rst briefly mentions that Pull-request: kdave#794 Author: rhn <[email protected]> [ rephrase changelog and copy text from pull request and add link to developer documentation of the send stream ] Signed-off-by: David Sterba <[email protected]>
…rdlink There's a report that newly added --rootdir print too many warnings for hardlinks, which is maybe not that uncommon. We still want to let the user know about that so print it just once and count how many were found: $ mkfs.btrfs --rootdir ... WARNING: '/tmp/btrfs-progs-mkfs-rootdir-hardlinks.7RcdfR/rootdir/inside_link' has extra hardlinks, they will be converted into new inodes WARNING: 12 hardlinks were detected in /tmp/btrfs-progs-mkfs-rootdir-hardlinks.7RcdfR/rootdir, all converted to new inodes Link: kdave#872 (comment) Signed-off-by: David Sterba <[email protected]>
Signed-off-by: David Sterba <[email protected]>
Signed-off-by: David Sterba <[email protected]>
Add new option --recursive 'btrfs subvol delete', causing it to pass the BTRFS_UTIL_DELETE_SUBVOLUME_RECURSIVE flag through to libbtrfsutil. This can work in two modes, depending on the user: - regular user - this will skip subvolumes that are not accessible - root (CAP_SYS_ADMIN) - no limitations Pull-request: kdave#861 Signed-off-by: Mark Harmstone <[email protected]> Co-authored-by: Omar Sandoval <[email protected]> Reviewed-by: Qu Wenruo <[email protected]> [ Add details to man page, fix indent in the doc. ] Signed-off-by: Qu Wenruo <[email protected]> Signed-off-by: David Sterba <[email protected]>
Add a new option --subvol, which tells mkfs.btrfs to create the specified directories as subvolumes when used with --rootdir. Given a populated directory dir, the command $ mkfs.btrfs --rootdir dir --subvol usr --subvol home --subvol home/username img will create subvolumes 'usr' and 'home' within the toplevel subvolume, and subvolume 'username' within the 'home' subvolume. It will fail if any of the directories do not yet exist. Pull-request: kdave#868 Reviewed-by: Qu Wenruo <[email protected]> Signed-off-by: Mark Harmstone <[email protected]> Signed-off-by: David Sterba <[email protected]>
|
Added to devel, thanks. |
kdave
pushed a commit
that referenced
this pull request
Aug 19, 2024
Remove last newline in the output of 'btrfs filesystem show', keep the line between two filesystems so the devices are visually grouped togehter. Pull-request: #866 Author: Matt Langford <[email protected]> Signed-off-by: David Sterba <[email protected]>
kdave
pushed a commit
that referenced
this pull request
Aug 19, 2024
Remove last newline in the output of 'btrfs filesystem show', keep the line between two filesystems so the devices are visually grouped togehter. Pull-request: #866 Author: Matt Langford <[email protected]> Signed-off-by: David Sterba <[email protected]>
kdave
pushed a commit
that referenced
this pull request
Sep 12, 2024
Remove last newline in the output of 'btrfs filesystem show', keep the line between two filesystems so the devices are visually grouped togehter. Pull-request: #866 Author: Matt Langford <[email protected]> Signed-off-by: David Sterba <[email protected]>
kdave
pushed a commit
that referenced
this pull request
Sep 17, 2024
Remove last newline in the output of 'btrfs filesystem show', keep the line between two filesystems so the devices are visually grouped togehter. Pull-request: #866 Author: Matt Langford <[email protected]> Signed-off-by: David Sterba <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes a stray newline in this command:
Each prior
pr_verbosealready includes a newline, so it doesn't appear to be needed. I've validated this patch removes that newline at the end (but keeps newlines between filesystem entries) when runningbtrfs filesystem showlocally.