Re: [PATCH v2 00/29] xfsprogs: Drop the 'platform_' prefix

Pavel Reichl
2021-08-07T15:13:40Z

On 8/7/21 1:05 AM, Dave Chinner wrote:
> On Fri, Aug 06, 2021 at 11:22:49PM +0200, Pavel Reichl wrote:
>> Stop using commands with 'platform_' prefix. Either use directly linux
>> standard command or drop the prefix from the function name.
> Looks like all of the patches in this series are missing
> signed-off-by lines. Most of them have empty commit messages, too,
Sorry about the missing signed-off-by, I really need to have a
check-list before posting patches...or send more patches :-).
> which we don't tend to do very often.
>
>> 51 files changed, 284 insertions(+), 151 deletions(-)
> IMO, 29 patches for such a small change is way too fine grained for
> working through efficiently. Empty commit messages tend to be a
> sign that you can aggregate patches together.... i.e. One patch for
> all the uuid changes (currently 7 patches) with a description of why
> you're changing the platform_uuid interface, one for all the mount
> related stuff (at least 5 patches now), one for all the block device
> stuff (8 or so patches), one for all the path bits, and then one for
> whatever is left over.
OK, I'll do this in the next version.
>
> Every patch has overhead, be it to produce, maintain, review, test,
> merge, etc. Breaking stuff down unnecessarily just increases the
> amount of work everyone has to do at every step. So if you find that
> you are writing dozens of patches that each have a trivial change in
> them that you are boiler-plating commit messages, you've probably
> made the overall changeset too fine grained.
OK, sincerely thank you for the 'rules-of-thump'. However, In the
first version of the patch set I grouped the changes into way less patches
and passed along a question about the preferred granularity of patches
and got the following answer:

> * What would be best for the reviewer - should I prepare a separate
> patch for every function rename or should I squash the changes into
> one huge patch?
> One patch per function, please.

However, I agree that I should have mentioned that some patches would
be too small and not blindly follow the request...I'll do better next
time.
>
> Also....
>
>> libxfs/init.c | 32 ++++++------
>> libxfs/libxfs_io.h | 2 +-
>> libxfs/libxfs_priv.h | 3 +-
>> libxfs/rdwr.c | 4 +-
>> libxfs/xfs_ag.c | 6 +--
>> libxfs/xfs_attr_leaf.c | 2 +-
>> libxfs/xfs_attr_remote.c | 2 +-
>> libxfs/xfs_btree.c | 4 +-
>> libxfs/xfs_da_btree.c | 2 +-
>> libxfs/xfs_dir2_block.c | 2 +-
>> libxfs/xfs_dir2_data.c | 2 +-
>> libxfs/xfs_dir2_leaf.c | 2 +-
>> libxfs/xfs_dir2_node.c | 2 +-
>> libxfs/xfs_dquot_buf.c | 2 +-
>> libxfs/xfs_ialloc.c | 4 +-
>> libxfs/xfs_inode_buf.c | 2 +-
>> libxfs/xfs_sb.c | 6 +--
>> libxfs/xfs_symlink_remote.c | 2 +-
> Why are all these libxfs files being changed?

I believe this is because of patch #6 - xfsprogs: Stop using
platform_uuid_copy()

Here I dropped the usage of platform_uuid_copy() even in libxfs by:

1) removing the uuid_copy() macro that was implemented by calling
platform_uuid_copy() in libxfs/libxfs_priv.h

-#define uuid_copy(s,d) platform_uuid_copy((s),(d))

2) using directly standard uuid_copy() instead
Which resulted into plenty of trivial changes all over libxfs, the
core of the changes being that uuid params are passed-by-value to
standard uuid_copy(), but to libxfs' uuid_copy() it is
passed-by-reference.

E.g.
- uuid_copy(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid);
+               uuid_copy(agf->agf_uuid, mp->m_sb.sb_meta_uuid);
> Cheers,
>
> Dave.
Hi Dave,
Thank you for you comments, please see reactions above.
Have a nice day.