On Fri, Jan 18, 2019 at 02:13:19PM +0800, Anand Jain wrote: > On 01/17/2019 11:54 PM, David Sterba wrote: > > On Thu, Jan 17, 2019 at 11:32:30PM +0800, Anand Jain wrote: > >> btrfs_find_device_by_path() is a helper function, drop the btrfs prefix > >> and the suffix _path is too generic, in fact as it reads superblock to > >> find the btrfs_device, so rename it to find_device_by_superblock() > > > > The function takes a path so it's search by path, as the name says, I > > don't think that needs to change. > > Sorry I didn't mention about my motivation to rename.. > find device by matching the device_path (without reading its > superblock) is done by btrfs_free_stale_devices() and its helper > device_path_matched() in the patch [1] which is in next-dev. > > [1] > btrfs: refactor btrfs_free_stale_devices() to get return value > > So a reader can think why not use btrfs_find_device_by_path() here, > unless fn is read to find out it matches the device by taking > the devid from the superblock. That would be really unfortunate to conflate a simple short helper that really wraps strcmp with a function doing a lot of other things. With source code at hand we don't have to speculate from a function name what it does, we go and read it if not sure. > Its not a big deal though.. I am ok to keep this as it is.. or... more > below.. > > > And even this is a helper, the > > btrfs_ prefix is useful in case there's something stuck inside the > > block layer functions that get called later so it's obvious on the > > stack. > > Makes sense. > > Or. > I think its a good idea to collapse this relatively small function into > its only parent, btrfs_find_device_by_devspec(). Devspec is either device id or the path, so the _devspec is a switch to two ways how to find the device and merging the the helper code does not seem to improve code. Sorry I really fail to see the point of this patch and the code issues it's supposed to resolve.
