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

[Sahil <icegambit91@gmail.com>]

Hi,

On Wednesday, March 13, 2024 9:17:44 PM IST Quentin Monnet wrote:
> Thanks! Apologies for the delay.

No worries! Thank you for the review.

> [...]
> Note: you don't need the blank lines between the tags.
> [...]
> You can keep the changelog as part of the patch description.

Got it. I'll keep this in mind when I submit v3.

> [...]
> With all the checks and the potential directory creation, we could maybe
> rename this into "prepare_bpffs_dir()" or something like this?

"prepare_bpffs_dir" is quite apt. If longer names are acceptable then I
would also recommend "prepare_and_mount_bpffs_dir" so it indicates
that it'll also mount the bpffs on the dir (when relevant) after performing
the checks.

> [...]
> I'd maybe change this block a little (although it's up to you):
> 
> 	bool dir_exists;
> 
> 	dire_exists = (access(...) == 0);
> 	if (!dir_exists) {
> 		...
> 		free(temp_name);
> 	}
> 
> 	if (block_mount) {
> 		...
> 	}
> 
> 	if (!dir_exists) {
> 		err = mkdir(...);
> 		...
> 	}
> 
> 	err = mnt_fs(...);
> 	...
> 
> This would also enable us to remove the directory we just created, if
> we're not able to mount the bpffs on it, before leaving the function.

I agree with this. This will also keep the implementation a little more
succinct with just one "block_mount" conditional block.

> [...]
> I'd remove/replace "given" from the name, maybe "mount_bpffs_for_file"?

This is definitely a better name. I am not very good when it comes to
making up names :P

> [...]
> I'd remove "file", the existing object might be a directory and it might
> be confusing.
> 
> > +		return -1;
> > +	}
> > +
> > +	temp_name = malloc(strlen(file_name) + 1);
> > +	if (!temp_name) {
> > 
> >  		p_err("mem alloc failed");
> >  		return -1;
> >  	
> >  	}
> > 
> > -	strcpy(file, name);
> > -	dir = dirname(file);
> > +	strcpy(temp_name, file_name);
> > +	dir = dirname(temp_name);
> 
> Here, we could check for the existence of "dir", and error out
> otherwise. The reason is that with the current code, if dir does not
> exist but user passes --nomount, then we fail to pin the path (given
> that the directory is not present), but the message returned will be "no
> BPF file system found, not mounting it due to --nomount option", which
> is confusing.
> 
> Same note applies to the other function as well.
> 
> >  	if (is_bpffs(dir))
> >  	
> >  		/* nothing to do if already mounted */
> > 
> > @@ -277,11 +333,11 @@ int mount_bpffs_for_pin(const char *name, bool
> > is_dir)> 
> >  	if (err) {
> >  	
> >  		err_str[ERR_MAX_LEN - 1] = '\0';
> >  		p_err("can't mount BPF file system to pin the object (%s): %s",
> 
> We could also indicate the path where we tried to mount the bpffs, in
> this message.

Understood. I'll make these changes as well.

> [...]
> Other than the comments above, the patch works well, and the different
> cases are much easier to follow than in v1, thanks!
> 
> I've checked that the programs load as expected, the directories are
> created (or not) as expected, and the bpffs is mounted (or not) as
> expected, for all the cases I could think of, with the following
> commands (copied here, for the record), all worked as I expected:

That's really nice to hear. I'll incorporate the recommended changes and
will send v3 soon.

Thanks,
Sahil