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

[Sahil <icegambit91@gmail.com>]

Hi,

Thank you for the reply.

On Monday, March 4, 2024 7:20:40 PM IST Quentin Monnet wrote:
> [...]
> Splitting the function was a suggestion, but you don't *have to* do it.
> What matters is the clarity of the resulting code, we want the function
> to be easy to follow and to not mix the file vs. directory paths too
> much (or then it's very easy to introduce bugs such as the existing one,
> or the missing --nomount check in your v1). Don't focus too much on
> malloc()/dirname() here, just make the logics easy to understand.

Understood, I'll refactor it accordingly.

> [...]
> If I understand correctly what you're asking, for files, "is_dir" would
> always evaluate to false so this check would be useless, wouldn't it? So
> yes we'd remove it.

This is the first part of my query.

> > If "is_bpffs(name)" returns false, then that could imply that the file
> > exists and its parent dir is not bpffs, or that the file does not exist
> > and no comment can be made on the parent dir. In either case the malloc
> > and dirname will have to be done.
> > 
> > On the other hand if the file exists
> 
> Note: We handle the case where a directory exists, not when the file
> itself already exists. If the file exists we get an error when trying to
> pin the program.

Right. And this is related to the second part of my query. If the file already
exists, an error should be thrown. But if it does not exist and the dir is not
bpffs, we'll end up doing:

    is_bpffs(name) // in the condition mentioned above
    [...]
    dir_name = dirname(name); // get parent dir name
    is_bpffs(dir_name) // call is_bpffs again
    [...]

So, in the "if" statement below:

    if (is_dir && is_bpffs(name))

will it be better to remove the second condition as well, and in lieu of that
we could simply run dirname() and is_bpffs(dir_name) if the function gets
a file as an argument?

Thanks,
Sahil