On Tue, Feb 06, 2024 at 04:21:15PM -0800, Boris Burkov wrote:
On Mon, Feb 05, 2024 at 06:43:39PM +0100, David Sterba wrote:
--- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -738,6 +738,7 @@ static noinline struct btrfs_device *device_list_add(const char *path, bool same_fsid_diff_dev = false; bool has_metadata_uuid = (btrfs_super_incompat_flags(disk_super) & BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
- bool can_create_new = *new_device_added;
It took me quite a while to figure out all the intended logic with the now in/out parameter. I think it's probably too cute? Why not just add another parameter "can_create_new_device"? I think it feels kind of weird on the caller side too, to set "new_device_added" to true, but then still rely on it to actually get set to true.
Once I got past this, the logic made sense, so definitely don't block yourself on this nit.
I had the separate parameter for the debugging version and removed for the final but I agree that it makes things less clear. Also there's something wrong with the device matching so the meaning of the parameter will be unchanged.