Re: [PATCH v5 1/7] fs: introduce kernel_pread_file* support

[Scott Branden]

Hi Luis,

A few comments inline before I cleanup.

On 2020-05-12 5:27 p.m., Luis Chamberlain wrote:
> On Thu, May 07, 2020 at 05:27:33PM -0700, Scott Branden wrote:
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 06b4c550af5d..cfab212fab9d 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -896,10 +896,14 @@ struct file *open_exec(const char *name)
> >   }
> >   EXPORT_SYMBOL(open_exec);
> >   
> > -int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > -		     loff_t max_size, enum kernel_read_file_id id)
> > -{
> > -	loff_t i_size, pos;
> > +int kernel_pread_file(struct file *file, void **buf, loff_t *size,
> > +		      loff_t pos, loff_t max_size, unsigned int flags,
> You use int flags, but.. these are mutually exclusive operations, and
> so flags is a misnomer. Just use an enum instead of a define, that way
> we can use kdoc for documentation.
OK, flags could be used to expand with additional flag options in the 
future (without change to function prototype, but will change to enum if 
that is what is desired.
> > +EXPORT_SYMBOL_GPL(kernel_pread_file);
> > +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path);
> > +EXPORT_SYMBOL_GPL(kernel_pread_file_from_path_initns);
> > +EXPORT_SYMBOL_GPL(kernel_pread_file_from_fd);
> If no one is using these don't export them. I think you only use one of
> these. In fact just remove the code from the ones which are not used.
I do not use them but added them to provide consistent api with 
kernel_read_file_* functions. That way someone can take advantage of 
the _from_path and from_fd variants in the future if desired. But if you 
want them removed it is simple to drop the EXPORT_SYMBOL_GPL and then 
add that back when first driver that calls them needs them in the future.

Note: Existing kernel_read_file_from_path_initns is not used in the 
kernel. Should we delete that as well?

>    Luis
Thanks,
ÂScott