Re: [PATCH 3/4] nvdimm: Add IOCTL pass thru

[Jerry Hoemann <jerry.hoemann@hpe.com>]

On Tue, Nov 10, 2015 at 04:45:16PM -0500, Jeff Moyer wrote:
> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
> 
> > On Tue, Nov 10, 2015 at 11:24:47AM -0500, Jeff Moyer wrote:
> >> Jerry Hoemann <jerry.hoemann@hpe.com> writes:
> >> 
> >> > @@ -633,10 +718,11 @@ static int match_dimm(struct device *dev, void *data)
> >> >  
> >> >  static long nvdimm_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >> >  {
> >> > -	int rc = -ENXIO, read_only;
> >> > +	int rc = -ENXIO, ro;
> >> >  	struct nvdimm_bus *nvdimm_bus;
> >> > +	unsigned int type = _IOC_TYPE(cmd);
> >> >  
> >> > -	read_only = (O_RDWR != (file->f_flags & O_ACCMODE));
> >> > +	ro = (O_RDWR != (file->f_flags & O_ACCMODE));
> >> 
> >> I'm still reviewing the rest of this, but this is bugging me.  The
> >> existing check for read_only looks pretty fishy to me.  O_WRONLY is a
> >> thing (even though it's probably not a supportable mode for this ioctl).
> >> Why not just check for O_RDONLY?
> >
> >
> > Good question.  I'll look into changing for version 2.
> > I suspect you would like something more like:
> >
> > 	ro = ((file->f_flags & O_ACCMODE) == O_RDONLY);
> 
> Yeah.  I'd make that a separate patch, and put it first in the series
> since it's a cleanup than can be applied to older kernels if necessary.
> 

  Will do.

-- 

-----------------------------------------------------------------------------
Jerry Hoemann            Software Engineer      Hewlett-Packard Enterprise

3404 E Harmony Rd. MS 36                        phone:  (970) 898-1022
Ft. Collins, CO 80528                           FAX:    (970) 898-0707
                                                email:  jerry.hoemann@hpe.com
-----------------------------------------------------------------------------