| Message ID | ae10e7c26537465368445d379c660fc8be62ad8b.1690541079.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | metadata_uuid misc cleanup and fixes part2 | expand |
On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> wrote: > > This change makes the code more readable. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/volumes.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 5678ca9b6281..4ce6c63ab868 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -833,14 +833,10 @@ static noinline struct btrfs_device *device_list_add(const char *path, > found_transid > fs_devices->latest_generation) { > memcpy(fs_devices->fsid, disk_super->fsid, > BTRFS_FSID_SIZE); > - > - if (has_metadata_uuid) > - memcpy(fs_devices->metadata_uuid, > - disk_super->metadata_uuid, > - BTRFS_FSID_SIZE); > - else > - memcpy(fs_devices->metadata_uuid, > - disk_super->fsid, BTRFS_FSID_SIZE); > + memcpy(fs_devices->metadata_uuid, > + has_metadata_uuid ? > + disk_super->metadata_uuid : disk_super->fsid, > + BTRFS_FSID_SIZE); While there's less lines of code, I don't find having a long ternary operation in the middle of a function call, split in two lines, more readable than the existing if-else statement, quite the contrary. Maybe I'm just being picky... Thanks. > > fs_devices->fsid_change = false; > } > -- > 2.38.1 >
On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote: > On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> wrote: > > > > This change makes the code more readable. > > > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > --- > > fs/btrfs/volumes.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > index 5678ca9b6281..4ce6c63ab868 100644 > > --- a/fs/btrfs/volumes.c > > +++ b/fs/btrfs/volumes.c > > @@ -833,14 +833,10 @@ static noinline struct btrfs_device *device_list_add(const char *path, > > found_transid > fs_devices->latest_generation) { > > memcpy(fs_devices->fsid, disk_super->fsid, > > BTRFS_FSID_SIZE); > > - > > - if (has_metadata_uuid) > > - memcpy(fs_devices->metadata_uuid, > > - disk_super->metadata_uuid, > > - BTRFS_FSID_SIZE); > > - else > > - memcpy(fs_devices->metadata_uuid, > > - disk_super->fsid, BTRFS_FSID_SIZE); > > + memcpy(fs_devices->metadata_uuid, > > + has_metadata_uuid ? > > + disk_super->metadata_uuid : disk_super->fsid, > > + BTRFS_FSID_SIZE); > > While there's less lines of code, I don't find having a long ternary > operation in the middle of a function call, split in two lines, more > readable than the existing if-else statement, quite the contrary. Agreed, one line of code doing one thing is readable.
On 29/07/2023 02:39, David Sterba wrote: > On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote: >> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> wrote: >>> >>> This change makes the code more readable. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> fs/btrfs/volumes.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>> index 5678ca9b6281..4ce6c63ab868 100644 >>> --- a/fs/btrfs/volumes.c >>> +++ b/fs/btrfs/volumes.c >>> @@ -833,14 +833,10 @@ static noinline struct btrfs_device *device_list_add(const char *path, >>> found_transid > fs_devices->latest_generation) { >>> memcpy(fs_devices->fsid, disk_super->fsid, >>> BTRFS_FSID_SIZE); >>> - >>> - if (has_metadata_uuid) >>> - memcpy(fs_devices->metadata_uuid, >>> - disk_super->metadata_uuid, >>> - BTRFS_FSID_SIZE); >>> - else >>> - memcpy(fs_devices->metadata_uuid, >>> - disk_super->fsid, BTRFS_FSID_SIZE); >>> + memcpy(fs_devices->metadata_uuid, >>> + has_metadata_uuid ? >>> + disk_super->metadata_uuid : disk_super->fsid, >>> + BTRFS_FSID_SIZE); >> >> While there's less lines of code, I don't find having a long ternary >> operation in the middle of a function call, split in two lines, more >> readable than the existing if-else statement, quite the contrary. > > Agreed, one line of code doing one thing is readable. My POV was one memcpy() per destination argument makes it better summarized at the function level. Anyway, I am okay with dropping this patch. Thanks.
On 29/07/2023 06:23, Anand Jain wrote: > > > On 29/07/2023 02:39, David Sterba wrote: >> On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote: >>> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> >>> wrote: >>>> >>>> This change makes the code more readable. >>>> >>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>>> --- >>>> fs/btrfs/volumes.c | 12 ++++-------- >>>> 1 file changed, 4 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>>> index 5678ca9b6281..4ce6c63ab868 100644 >>>> --- a/fs/btrfs/volumes.c >>>> +++ b/fs/btrfs/volumes.c >>>> @@ -833,14 +833,10 @@ static noinline struct btrfs_device >>>> *device_list_add(const char *path, >>>> found_transid > fs_devices->latest_generation) { >>>> memcpy(fs_devices->fsid, disk_super->fsid, >>>> BTRFS_FSID_SIZE); >>>> - >>>> - if (has_metadata_uuid) >>>> - memcpy(fs_devices->metadata_uuid, >>>> - disk_super->metadata_uuid, >>>> - BTRFS_FSID_SIZE); >>>> - else >>>> - memcpy(fs_devices->metadata_uuid, >>>> - disk_super->fsid, >>>> BTRFS_FSID_SIZE); >>>> + memcpy(fs_devices->metadata_uuid, >>>> + has_metadata_uuid ? >>>> + disk_super->metadata_uuid : >>>> disk_super->fsid, >>>> + BTRFS_FSID_SIZE); >>> >>> While there's less lines of code, I don't find having a long ternary >>> operation in the middle of a function call, split in two lines, more >>> readable than the existing if-else statement, quite the contrary. >> >> Agreed, one line of code doing one thing is readable. > > My POV was one memcpy() per destination argument makes it better > summarized at the function level. Anyway, I am okay with dropping > this patch. > I missed something. I have a helper function btrfs_sb_fsid_ptr() in the patch 3/4 which reads either metadata_uuid or fsid as per METADATA_UUID flag. Which in that case the code shall be memcpy(fs_devices->metadata_uuid, btrfs_sb_fsid_ptr(disk_super), BTRFS_FSID_SIZE); I think this is better? > Thanks.
On 29/7/23 12:22, Anand Jain wrote: > On 29/07/2023 06:23, Anand Jain wrote: >> >> >> On 29/07/2023 02:39, David Sterba wrote: >>> On Fri, Jul 28, 2023 at 06:40:39PM +0100, Filipe Manana wrote: >>>> On Fri, Jul 28, 2023 at 5:43 PM Anand Jain <anand.jain@oracle.com> >>>> wrote: >>>>> >>>>> This change makes the code more readable. >>>>> >>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>>>> --- >>>>> fs/btrfs/volumes.c | 12 ++++-------- >>>>> 1 file changed, 4 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>>>> index 5678ca9b6281..4ce6c63ab868 100644 >>>>> --- a/fs/btrfs/volumes.c >>>>> +++ b/fs/btrfs/volumes.c >>>>> @@ -833,14 +833,10 @@ static noinline struct btrfs_device >>>>> *device_list_add(const char *path, >>>>> found_transid > fs_devices->latest_generation) { >>>>> memcpy(fs_devices->fsid, disk_super->fsid, >>>>> BTRFS_FSID_SIZE); >>>>> - >>>>> - if (has_metadata_uuid) >>>>> - memcpy(fs_devices->metadata_uuid, >>>>> - disk_super->metadata_uuid, >>>>> - BTRFS_FSID_SIZE); >>>>> - else >>>>> - memcpy(fs_devices->metadata_uuid, >>>>> - disk_super->fsid, >>>>> BTRFS_FSID_SIZE); >>>>> + memcpy(fs_devices->metadata_uuid, >>>>> + has_metadata_uuid ? >>>>> + disk_super->metadata_uuid : >>>>> disk_super->fsid, >>>>> + BTRFS_FSID_SIZE); >>>> >>>> While there's less lines of code, I don't find having a long ternary >>>> operation in the middle of a function call, split in two lines, more >>>> readable than the existing if-else statement, quite the contrary. >>> >>> Agreed, one line of code doing one thing is readable. >> >> My POV was one memcpy() per destination argument makes it better >> summarized at the function level. Anyway, I am okay with dropping >> this patch. >> > > I missed something. I have a helper function btrfs_sb_fsid_ptr() in the > patch 3/4 which reads either metadata_uuid or fsid as per METADATA_UUID > flag. Which in that case the code shall be > > memcpy(fs_devices->metadata_uuid, btrfs_sb_fsid_ptr(disk_super), > BTRFS_FSID_SIZE); > > I think this is better? Sent v2 with this changed. Thx. > >> Thanks. >
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 5678ca9b6281..4ce6c63ab868 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -833,14 +833,10 @@ static noinline struct btrfs_device *device_list_add(const char *path, found_transid > fs_devices->latest_generation) { memcpy(fs_devices->fsid, disk_super->fsid, BTRFS_FSID_SIZE); - - if (has_metadata_uuid) - memcpy(fs_devices->metadata_uuid, - disk_super->metadata_uuid, - BTRFS_FSID_SIZE); - else - memcpy(fs_devices->metadata_uuid, - disk_super->fsid, BTRFS_FSID_SIZE); + memcpy(fs_devices->metadata_uuid, + has_metadata_uuid ? + disk_super->metadata_uuid : disk_super->fsid, + BTRFS_FSID_SIZE); fs_devices->fsid_change = false; }
This change makes the code more readable. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/volumes.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)