Re: JFFS2: misc. error cases

[Andrew Lunn <andrew at lunn dot ch>]

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.

> 
> 
> 
> --- dir-ecos.c	2003-12-12 00:33:54.000000000 +0100
> +++ mydir-ecos.c	2005-02-03 13:53:09.807444000 +0100
> @@ -48,9 +48,9 @@
>  	up(&dir_f->sem);
>  	if (ino) {
>  		inode = jffs2_iget(dir_i->i_sb, ino);
> -		if (!inode) {
> +		if (IS_ERR(inode)) {
>  			printk("jffs2_iget() failed for ino #%u\n", ino);
> -			return (ERR_PTR(-EIO));
> +			return inode;
>  		}
>  	}
>  
> 
> --- fs-ecos.c	2005-01-22 18:14:49.000000000 +0100
> +++ myfs-ecos.c	2005-02-03 13:50:02.246297000 +0100
> @@ -825,9 +825,11 @@
>  	} else {
>  		// If there we no error, something already exists with that
>  		// name, so we cannot create another one.
> -		jffs2_iput(ds.node);
>  		if (err == ENOERR)
> +		{
>  			err = EEXIST;
> +			jffs2_iput(ds.node); // don't iput if not found !
> +		}
>  	}
>  	jffs2_iput(ds.dir);
>  	return err;
> @@ -946,7 +948,6 @@
>  		    ds2.dir->i_ctime = ds2.dir->i_mtime = cyg_timestamp();
>   out:
>  	jffs2_iput(ds1.dir);
> -	jffs2_iput(ds1.node);
>  	if (S_ISDIR(ds1.node->i_mode)) {
>  		/* Renamed a directory to elsewhere... so fix up its
>  		   i_parent pointer and the i_counts of its old and
> @@ -957,10 +958,11 @@
>  	} else {
>  		jffs2_iput(ds2.dir); /* ... doing this */
>  	}
> +	jffs2_iput(ds1.node); /* iput this one AFTER the S_IFDIR test ;) */
>  	if (ds2.node)
>  		jffs2_iput(ds2.node);
>   
> -	return -err;
> +	return err;
>  }
>  // -------------------------------------------------------------------------
> @@ -1096,7 +1098,10 @@
>  
>  		// check it is a directory
>  		if (!S_ISDIR(ds.node->i_mode))
> +		{
> +			jffs2_iput(ds.node);
>  			return ENOTDIR;
> +		}
>  
>  		// Pass it out
>  		*dir_out = (cyg_dir) ds.node;
> @@ -1363,7 +1368,7 @@
>  		int err;
>  
>  		D2(printf("jffs2_fo_write page_start_pos %d\n", pos));
> -		D2(printf("jffs2_fo_write transfer size %d\n", l));
> +		D2(printf("jffs2_fo_write transfer size %d\n", len));
>  
>  		err = jffs2_write_inode_range(c, f, &ri, buf,
>  					      pos, len, &writtenlen);

Everything upto here is OK.

> @@ -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.

>  
>  	// 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.

>  
>  //==========================================================================
> @@ -1752,13 +1757,14 @@
>  	// Not cached, so malloc it
>  	inode = new_inode(sb);
>  	if (inode == NULL)
> -		return 0;
> +		return ERR_PTR(-ENOMEM);
>  
>  	inode->i_ino = ino;

OK

>  
>  	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);
> @@ -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;

Please could you explain these two in more detail. Why is the change
needed etc...

        Thanks
                Andrew