On Mon, Oct 7, 2024 at 4:39 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Mon, Oct 07, 2024 at 04:19:21PM +0200, Amir Goldstein wrote:
> > +static int ovl_upper_fdget(const struct file *file, struct fd *real, bool data)
> > +{
> > + struct dentry *dentry = file_dentry(file);
> > + struct path realpath;
> > + enum ovl_path_type type;
> > +
> > + if (data)
> > + type = ovl_path_realdata(dentry, &realpath);
> > + else
> > + type = ovl_path_real(dentry, &realpath);
> > +
> > + real->word = 0;
> > + /* Not interested in lower nor in upper meta if data was requested */
> > + if (!OVL_TYPE_UPPER(type) || (data && OVL_TYPE_MERGE(type)))
> > + return 0;
> > +
> > + return ovl_real_fdget_path(file, real, &realpath);
> > }
> >
> > static int ovl_open(struct inode *inode, struct file *file)
> > @@ -394,16 +411,14 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> > if (ret <= 0)
> > return ret;
> >
> > - ret = ovl_real_fdget_meta(file, &real, !datasync);
> > - if (ret)
> > + /* Don't sync lower file for fear of receiving EROFS error */
> > + ret = ovl_upper_fdget(file, &real, datasync);
> > + if (ret || fd_empty(real))
> > return ret;
>
> Is there any real point in keeping ovl_upper_fdget() separate from the
> only caller? Note that the checks for type make a lot more sense
> in ovl_fsync() than buried in a separate helper and this fd_empty()
> is a "do we have the wrong type?" check in disguise.
>
> Why not just expand it at the call site?
You are right (again) I open code it, it looks much better:
/* Don't sync lower file for fear of receiving EROFS error */
- upperfile = ovl_upper_file(file, datasync);
- if (IS_ERR_OR_NULL(upperfile))
+ type = ovl_path_type(dentry);
+ if (!OVL_TYPE_UPPER(type) || (datasync && OVL_TYPE_MERGE(type)))
+ return 0;
+
+ ovl_path_upper(dentry, &upperpath);
+ upperfile = ovl_real_file_path(file, &upperpath);
+ if (IS_ERR(upperfile))
return PTR_ERR(upperfile);
Thanks,
Amir.
