Re: [PATCH bpf-next] bpftool: Mount bpffs on provided dir instead of parent dir

[Sahil <icegambit91@gmail.com>]

Hi,

Thank you for the review.

On Thursday, February 29, 2024 8:29:07 PM IST Quentin Monnet wrote:
> [...]
> The error string should be updated, we're not trying to pin one object
> file here but to mount the bpffs on a directory to pin several objects.

Sorry, I forgot to change the error message here. I'll change it.

> > +                               name, err_str);
> 
> Formatting nit: "name" should be aligned with the argument from the line
> above (the opening double quote). You can catch this by running
> "./scripts/checkpatch.pl --strict" on your patch/commit.

Got it. I ran the checkpatch script without --strict, so it didn't catch this.

> > +		}
> > +
> > +		return err;
> > +	}
> 
> This block above cannot be before the check on "block_mount", or we will
> ignore the "--nomount" option if the user passes it.

Oh, I understand this now. The block_mount check should be done before any
attempt to mount the bpffs.

> Perhaps it would be clearer to split the logics of mount_bpffs_for_pin()
> into two subfunctions, one for directories, one for file paths. This way
> we would avoid to call malloc() and dirname() when "name" is already a
> directory, and it would be easier to follow the different cases.

I agree. I am thinking of having two mount_bpffs_for_pin_* functions, one for dirs
and one for files. These will handle the differences between dirs and files and they
can both call a third (but static) mount_bpffs_for_pin where the code common to both
scenarios will exist. The actual mounting and --nomount check can be done in this
static function.

> [...]
> We use err_str to pass it to mnt_fs, but we cannot use it here (it is
> not set by mkdir). We probably want "strerror(errno)" instead.

Understood. I'll change this too.

Thanks,
Sahil