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
