Re: [PATCH v4 4/6] btrfs: handle device lookup with btrfs_dev_lookup_args

[David Sterba <dsterba@suse.cz>]

On Wed, Oct 06, 2021 at 10:34:41AM +0300, Nikolay Borisov wrote:
> >  int btrfs_init_dev_replace(struct btrfs_fs_info *fs_info)
> >  {
> > +	BTRFS_DEV_LOOKUP_ARGS(args);
> 
> nit: This line can be:
> 
> struct btrfs_dev_lookup_args args = {.devid = BTRFS_DEV_REPLACE_DEVID};
> 
> as it doesn't go over the 76 char limit, the only reason I'm suggesting

The limit is 80-85.

> it is for consistency sake, since in
> btrfs_scrub_progress/btrfs_scrub_dev the latter style is preferred.

Agreed, after seeing the rest of the patchset, if the members (most
often devid) can be set right away like from the parameters, the
explicit initialization should be used, so I've switched it to what
you've suggested.

> > +static inline bool dev_args_match_fs_devices(struct btrfs_dev_lookup_args *args,

Btw static inline that's not in a header does not make much sense, so
it's plain static. Also for read-only parameters it's a good habit to
declare them const, also changed.

> > +					     struct btrfs_fs_devices *fs_devices)
> > +{
> > +	if (args->fsid == NULL)
> > +		return true;
> > +	if (!memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE))
> > +		return true;
> > +	return false;
> 
> Make last 3 lines into:
> 
> return !memcmp(fs_devices->metadata_uuid, args->fsid, BTRFS_FSID_SIZE)

I really don't like the short form where memcmp (or strcmp for that
matter) is done like !memcmp as it reads "if the don't compare as
equeal" and requires the mental switch to if memcmp() == 0 "they're
equal", so I've been switching that to the == 0 form whenever I see it.

In this function, if ther's a chain of if/if/return, it reads better if
all the branches are either implicit or explicit, so mixing both styles
is slightly less preferred.

> > @@ -517,7 +530,7 @@ int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64 len);
> >  int btrfs_grow_device(struct btrfs_trans_handle *trans,
> >  		      struct btrfs_device *device, u64 new_size);
> >  struct btrfs_device *btrfs_find_device(struct btrfs_fs_devices *fs_devices,
> > -				       u64 devid, u8 *uuid, u8 *fsid);
> > +				       struct btrfs_dev_lookup_args *args);
> 
> Let's annotate this pointer with const to be more explicit about this
> being really an input-only struct.

Agreed, const added.