On Sat, Oct 5, 2024 at 12:23 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 04, 2024 at 12:23:41PM +0200, Amir Goldstein wrote:
>
> > if (upper_meta) {
> > ovl_path_upper(dentry, &realpath);
> > if (!realpath.dentry)
> > - return 0;
> > + return NULL;
> > } else {
> > /* lazy lookup and verify of lowerdata */
> > err = ovl_verify_lowerdata(dentry);
> > if (err)
> > - return err;
> > + return ERR_PTR(err);
>
> Ugh... That kind of calling conventions is generally a bad idea.
>
> > + return realfile;
>
> ... especially since it's NULL/ERR_PTR()/pointer to object.
>
>
> > + realfile = ovl_real_file_meta(file, !datasync);
> > + if (IS_ERR_OR_NULL(realfile))
> > + return PTR_ERR(realfile);
>
> Please, don't. IS_ERR_OR_NULL is bogus 9 times out of 10 (at least).
IDK, we have quite a few of these constants in ovl code and it's pretty
clear and useful to my taste, but I am open to being corrected.
Anyway, I pushed a new version to
https://github.com/amir73il/linux/commits/ovl_real_file-v2/
Where we have:
- ovl_dir_real_file() and ovl_upper_file() can return NULL and their
few callers check for IS_ERR_OR_NULL()
> And you still have breakage in llseek et.al.
- ovl_real_file() and ovl_real_file_path() cannot return NULL and all
their callers (llseek et.al.) check only for IS_ERR()
Let me know if you think this is still problematic.
Thanks,
Amir.
