2024-03-04 20:34 UTC+0000 ~ Sahil <icegambit91@xxxxxxxxx> > 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? Yes, we should remove this second condition too for files. Running "is_bpffs(name)" makes no sense as we know we'll have an error at this point. What we could do to improve the current code, however, is returning an error from mount_bpffs_for_pin() if the file exists, rather than mounting the bpffs and waiting for bpf_obj_pin() to return the error. This would prevent bpftool from mounting the bpffs when we already know the operation will fail. Quentin
