This is the mail archive of the ecos-patches@sources.redhat.com mailing list for the eCos project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: JFFS2: misc. error cases


Andrew Lunn wrote:
> 
> On Thu, Feb 03, 2005 at 02:32:49PM +0100, Estelle HAMMACHE wrote:
> > Hello,
> >
> > here are a few corrections (I think) of the
> > eCos-specific part of JFFS2. These are mainly
> > error case problems that I have encountered
> > during coverage tests.
> >
> > Estelle
> 
> Hi Estelle.
> 
> Most of this patch i agree with. But there are some bits i think might
> be wrong and some bits i would like explaining because they are not
> obvious to me.
> 

Hi Andrew,

my comments below for the cases which required explanation.

Additionally I forgot an error code sign modification on line 712:

			err = jffs2_create(ds.dir, ds.name, S_IRUGO|S_IXUGO|S_IWUSR|S_IFREG, &node);

			if (err != 0) {
                                //Possible orphaned inode on the flash - but will be gc'd
                          	jffs2_iput(ds.dir);
+				return -err;
-                                return err;
			}

			err = ENOERR;
		}
#endif




> > @@ -1423,7 +1428,7 @@
> >
> >          // Check that pos is still within current file size, or at the
> >          // very end.
> > -        if (pos < 0 )
> > +        if (pos < 0 || pos > node->i_size)
> >                  return EINVAL;
> 
> This i think is wrong. You are allowed to seek past the end of the
> file. Thats how you make holes. I think the code is right and the
> comment is wrong.

That's possible. Let's drop this change, or rather maybe update the
comment. I think it is actually a regression on a previous patch 
by someone else.


> 
> >
> >       // All OK, set fp offset and return new position.
> > @@ -1539,7 +1544,7 @@
> >
> >       D2(printf("jffs2_fo_setinfo\n"));
> >
> > -     return ENOERR;
> > +     return EINVAL;
> >  }
> 
> This i also think is wrong. If you have reached the end of the
> function everything has gone OK and you have sucess. So you should
> return ENOERR.

It looks inconsistent that jffs2_setinfo returns EINVAL in this case
and that jffs2_fo_setinfo should return ENOERR.
Plus, the corresponding getinfo functions return EINVAL if the key
is not supported, which is the case here. The function does not
really do what the user expects, so returning ENOERR is misleading...


> >
> >       err = jffs2_read_inode(inode);
> >       if (err) {
> >               printf("jffs2_read_inode() failed\n");
> > +             inode->i_nlink = 0; // free _this_ bad inode right now
> >               jffs2_iput(inode);
> >               inode = NULL;
> >               return ERR_PTR(err);

The same hack is done several times in dir-ecos.c.
If i_nlink is not 0 (new_inode() initialized it to 1), 
jffs2_iput may leave the inode in the cache. 
But jffs2_read_inode failed to build the 
node tree (or other problem). So the inode is bad 
and should be evicted from the cache right now, so 
that another open will not attempt to use the badly 
initialized inode which was left in the cache 
previously.
If there was a memory shortage, trying to read the
inode (build the inode tree) later may succeed...
If it was an IO error it may fail again, but there
will be no half-initialized things remaining that
may cause a segfault.


> > @@ -1869,7 +1875,16 @@
> >       ri->mode =  cpu_to_jemode(mode);
> >       ret = jffs2_do_new_inode (c, f, mode, ri);
> >       if (ret) {
> > -             jffs2_iput(inode);
> > +             // forceful evict: f->sem is locked already,
> > +             // and the inode is bad.
> > +             if (inode->i_cache_prev)
> > +                     inode->i_cache_prev->i_cache_next = inode->i_cache_next;
> > +             if (inode->i_cache_next)
> > +                     inode->i_cache_next->i_cache_prev = inode->i_cache_prev;
> > +             up(&(f->sem));
> > +             jffs2_clear_inode(inode);
> > +             memset(inode, 0x6a, sizeof(*inode));
> > +             free(inode);
> >               return ERR_PTR(ret);
> >       }
> >       inode->i_nlink = 1;
> 

The previous code merely called jffs2_iput. This causes trouble
because
- inode->nlink was initialized to 1 by new_inode() (same problem
  as above - the inode might stay in the cache even though it was
  not correctly filled)
- f->sem was initialized in locked state by jffs2_init_inode_info()
  and this will cause a deadlock in jffs2_do_clear_inode()
  called by jffs2_clear_inode(), called by jffs2_iput().
- possibly other things that I did not see
So, rather than attempting to set everything right so that 
jffs2_iput() will work, I preferred to forcefully evict the inode
from the cache directly. Of course you may choose to do it in
some other way, but the current code does not work.

bye
Estelle


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]