Re: [PATCH 3/4] mtd: Introduce mtd_block_isreserved()

[Brian Norris <computersforpeace@gmail.com>]

On Wed, May 14, 2014 at 08:37:21PM -0300, Ezequiel Garcia wrote:
> On 12 May 06:31 PM, Brian Norris wrote:
> > On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> [..]
> > >  
> > > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > > +{
> > > +	if (!mtd->_block_isreserved)
> > > +		return 0;
> > > +	if (ofs < 0 || ofs > mtd->size)
> > > +		return -EINVAL;
> > 
> > At first, I was going to recommend that the out-of-bounds check go
> > before the !mtd->_block_isreserved check, since it's best to warn users
> > for invalid input. But then, mtd_block_isbad() has the same ordering, so
> > it'd be nice to consistent...
> > 
> > Do we flip a coin to decide whether to change both or leave as-is? :)
> > 
> 
> Actually, I just followed the same convention as all the other functions,
> not just mtd_block_isbad().

All? Like what? mtd_read_oob()? mtd_get_fact_prot_info()? These return
-EOPNOTSUPP, which is an informative error code. But that's different
than returning 0 for mtd_block_isbad() or mtd_block_isreserved(), even
if the block doesn't exist.

> I'll add a patch changing them all so the parameters checking is done first.

Can you mention which ones you think are problematic before adding
another patch? I'd be careful on playing with error codes for APIs that
are already have reasonable enough error codes. AFAICT,
mtd_block_isbad() (and your mtd_block_isreserved()) are the only
problems.

(Also, is it just me, or is mtd_writev() missing bounds checking?)

Brian