| Message ID | 718713677855382e44cb57d1ad590063ca20d8f7.1686202417.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: cleanup and preparatory around device scan | expand |
On 2023/6/8 14:01, Anand Jain wrote: > Local variable int ret is unnecessary, drop it. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> I'm not sure if this change is really necessary. To me it's more nature to go "ret = what_ever();" then handling @ret. And compiler is definitely clever enough for those optimization. Thanks, Qu > --- > kernel-shared/volumes.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c > index 81abda3f7d1c..c8053ae1c7f7 100644 > --- a/kernel-shared/volumes.c > +++ b/kernel-shared/volumes.c > @@ -545,10 +545,8 @@ int btrfs_scan_one_device(int fd, const char *path, > u64 *total_devs, u64 super_offset, unsigned sbflags) > { > struct btrfs_super_block disk_super; > - int ret; > > - ret = btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags); > - if (ret < 0) > + if (btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags) < 0) > return -EIO; > > if (btrfs_super_flags(&disk_super) & BTRFS_SUPER_FLAG_METADUMP) > @@ -556,9 +554,7 @@ int btrfs_scan_one_device(int fd, const char *path, > else > *total_devs = btrfs_super_num_devices(&disk_super); > > - ret = device_list_add(path, &disk_super, fs_devices_ret); > - > - return ret; > + return device_list_add(path, &disk_super, fs_devices_ret); > } > > static u64 dev_extent_search_start(struct btrfs_device *device, u64 start)
On Thu, Jun 08, 2023 at 02:22:35PM +0800, Qu Wenruo wrote: > > > On 2023/6/8 14:01, Anand Jain wrote: > > Local variable int ret is unnecessary, drop it. > > > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > I'm not sure if this change is really necessary. > > To me it's more nature to go "ret = what_ever();" then handling @ret. > And compiler is definitely clever enough for those optimization. Yeah we do ret = function(...) if (ret < 0) ... almost everywhere. I find it clear, there's something happening is on a separate line and the condtion check should have as little side effects as possible.
On 08/06/2023 20:42, David Sterba wrote: > On Thu, Jun 08, 2023 at 02:22:35PM +0800, Qu Wenruo wrote: >> >> >> On 2023/6/8 14:01, Anand Jain wrote: >>> Local variable int ret is unnecessary, drop it. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> I'm not sure if this change is really necessary. >> >> To me it's more nature to go "ret = what_ever();" then handling @ret. >> And compiler is definitely clever enough for those optimization. > > Yeah we do > > ret = function(...) > if (ret < 0) > ... > > almost everywhere. I find it clear, there's something happening is > on a separate line and the condtion check should have as little side > effects as possible. I noticed that the partner will leave the ret to stay. I am okay with dropping this patch in the next reroll. I assume you also meant the following is not needed? @@ -556,9 +554,7 @@ int btrfs_scan_one_device(int fd, const char *path, else *total_devs = btrfs_super_num_devices(&disk_super); - ret = device_list_add(path, &disk_super, fs_devices_ret); - - return ret; + return device_list_add(path, &disk_super, fs_devices_ret); Thanks
diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c index 81abda3f7d1c..c8053ae1c7f7 100644 --- a/kernel-shared/volumes.c +++ b/kernel-shared/volumes.c @@ -545,10 +545,8 @@ int btrfs_scan_one_device(int fd, const char *path, u64 *total_devs, u64 super_offset, unsigned sbflags) { struct btrfs_super_block disk_super; - int ret; - ret = btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags); - if (ret < 0) + if (btrfs_read_dev_super(fd, &disk_super, super_offset, sbflags) < 0) return -EIO; if (btrfs_super_flags(&disk_super) & BTRFS_SUPER_FLAG_METADUMP) @@ -556,9 +554,7 @@ int btrfs_scan_one_device(int fd, const char *path, else *total_devs = btrfs_super_num_devices(&disk_super); - ret = device_list_add(path, &disk_super, fs_devices_ret); - - return ret; + return device_list_add(path, &disk_super, fs_devices_ret); } static u64 dev_extent_search_start(struct btrfs_device *device, u64 start)
Local variable int ret is unnecessary, drop it. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- kernel-shared/volumes.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)