[PATCH] separate routine to check jffs2_flash_read

[PATCH] separate routine to check jffs2_flash_read

[Pierre.Ricadat@UTBM.fr]

[-- Attachment #1: Type: text/plain, Size: 473 bytes --]

Hi all'

here i attach a small patch for that 'todo' in nodelist.c:

  /* TODO: this is very frequent pattern, make it a separate
		   * routine */
  err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
  ...

about the checking of jffs2_flash_read().

I made a new function : jffs2_check_flash_read to
- check that jffs2_flash_read returns success
- check that it read all
and in a case of failure, print error, free the buffer and return error code.


-- 
Pierre Ricadat

[-- Attachment #2: jffs2_check_flash_read.patch --]
[-- Type: text/plain, Size: 11064 bytes --]

diff -u ./mtd/fs/jffs2/debug.c ./mtd_pierre/fs/jffs2/debug.c
--- ./mtd/fs/jffs2/debug.c      2005-09-21 22:28:35.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/debug.c       2005-10-24 17:03:25.000000000 +0900
@@ -131,12 +131,8 @@
                return;

        ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
-       if (ret || (retlen != len)) {
-               JFFS2_WARNING("read %d bytes failed or short. ret %d, retlen %zd.\n",
-                               len, ret, retlen);
-               kfree(buf);
+       if (!jffs2_check_flash_read(ret, ofs, len, retlen, &buf))
                return;
-       }

        ret = 0;
        for (i = 0; i < len; i++)
@@ -620,11 +616,8 @@
        printk(JFFS2_DBG_MSG_PREFIX " dump node at offset %#08x.\n", ofs);

        ret = jffs2_flash_read(c, ofs, len, &retlen, (unsigned char *)&node);
-       if (ret || (retlen != len)) {
-               JFFS2_ERROR("read %d bytes failed or short. ret %d, retlen %zd.\n",
-                       len, ret, retlen);
+       if (!jffs2_check_flash_read(ret, ofs, len, retlen, (unsigned char *)&node))
                return;
-       }

        printk(JFFS2_DBG "magic:\t%#04x\n", je16_to_cpu(node.u.magic));
        printk(JFFS2_DBG "nodetype:\t%#04x\n", je16_to_cpu(node.u.nodetype));

diff -u ./mtd/fs/jffs2/erase.c ./mtd_pierre/fs/jffs2/erase.c
--- ./mtd/fs/jffs2/erase.c      2005-09-20 23:53:15.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/erase.c       2005-10-24 17:12:57.000000000 +0900
@@ -323,14 +323,8 @@
                *bad_offset = ofs;

                ret = jffs2_flash_read(c, ofs, readlen, &retlen, ebuf);
-               if (ret) {
-                       printk(KERN_WARNING "Read of newly-erased block at 0x%08x failed: %d. Putting on bad_list\n", ofs, ret);
-                       goto fail;
-               }
-               if (retlen != readlen) {
-                       printk(KERN_WARNING "Short read from newly-erased block at 0x%08x. Wanted %d, got %zd\n", ofs, readlen, retlen);
-                       goto fail;
-               }
+               if (!jffs2_check_flash_read(ret, ofs, readlen, retlen, (void *)&ebuf))
+                       return ret;
                for (i=0; i<readlen; i += sizeof(unsigned long)) {
                        /* It's OK. We know it's properly aligned */
                        unsigned long *datum = ebuf + i;

diff -u ./mtd/fs/jffs2/gc.c ./mtd_pierre/fs/jffs2/gc.c
--- ./mtd/fs/jffs2/gc.c 2005-09-07 17:34:54.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/gc.c  2005-10-25 14:42:04.000000000 +0900
@@ -531,10 +531,8 @@
                return -ENOMEM;

        ret = jffs2_flash_read(c, ref_offset(raw), rawlen, &retlen, (char *)node);
-       if (!ret && retlen != rawlen)
-               ret = -EIO;
-       if (ret)
-               goto out_node;
+       if (!jffs2_check_flash_read(ret, ref_offset(raw), rawlen, retlen, (char *)&node))
+               return ret;

        crc = crc32(0, node, sizeof(struct jffs2_unknown_node)-4);
        if (je32_to_cpu(node->u.hdr_crc) != crc) {
@@ -853,16 +851,9 @@
                        /* This is an obsolete node belonging to the same directory, and it's of the right
                           length. We need to take a closer look...*/
                        ret = jffs2_flash_read(c, ref_offset(raw), rawlen, &retlen, (char *)rd);
-                       if (ret) {
-                               printk(KERN_WARNING "jffs2_g_c_deletion_dirent(): Read error (%d) reading obsolete node at %08x\n", ret, ref_offset(raw));
+                       if (!jffs2_check_flash_read(ret, ref_offset(raw), rawlen, retlen, (char *)rd))
                                /* If we can't read it, we don't need to continue to obsolete it. Continue */
                                continue;
-                       }
-                       if (retlen != rawlen) {
-                               printk(KERN_WARNING "jffs2_g_c_deletion_dirent(): Short read (%zd not %u) reading header from obsolete node at %08x\n",
-                                      retlen, rawlen, ref_offset(raw));
-                               continue;
-                       }

                        if (je16_to_cpu(rd->nodetype) != JFFS2_NODETYPE_DIRENT)
                                continue;

diff -u ./mtd/fs/jffs2/nodelist.c ./mtd_pierre/fs/jffs2/nodelist.c
--- ./mtd/fs/jffs2/nodelist.c   2005-09-21 22:28:35.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/nodelist.c    2005-10-24 17:17:24.000000000 +0900
@@ -451,19 +451,9 @@
                if (unlikely(!buffer))
                        return -ENOMEM;

-               /* TODO: this is very frequent pattern, make it a separate
-                * routine */
                err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
-               if (err) {
-                       JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
-                       goto free_out;
-               }
-
-               if (retlen != len) {
-                       JFFS2_ERROR("short read at %#08x: %d instead of %d.\n", ofs, retlen, len);
-                       err = -EIO;
-                       goto free_out;
-               }
+               if (!jffs2_check_flash_read(err, ofs, len, retlen, (unsigned char*)&buffer))
+                       return err;
        }

        /* Continue calculating CRC */
@@ -498,14 +488,6 @@

        return 0;

-free_out:
-       if(!pointed)
-               kfree(buffer);
-#ifndef __ECOS
-       else
-               c->mtd->unpoint(c->mtd, buffer, ofs, len);
-#endif
-       return err;
 }

diff -u ./mtd/fs/jffs2/os-linux.h ./mtd_pierre/fs/jffs2/os-linux.h
--- ./mtd/fs/jffs2/os-linux.h   2005-09-30 22:59:13.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/os-linux.h    2005-10-24 19:03:35.000000000 +0900
@@ -122,6 +122,7 @@
 int jffs2_flash_writev(struct jffs2_sb_info *c, const struct kvec *vecs, unsigned long count, loff_t to, size_t *retlen, uint32_t ino);
 int jffs2_flash_write(struct jffs2_sb_info *c, loff_t ofs, size_t len, size_t *retlen, const u_char *buf);
 int jffs2_flash_read(struct jffs2_sb_info *c, loff_t ofs, size_t len, size_t *retlen, u_char *buf);
+int jffs2_check_flash_read(int err, uint32_t ofs, int len, size_t retlen, u_char *buf);
 int jffs2_check_oob_empty(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,int mode);
 int jffs2_check_nand_cleanmarker(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
 int jffs2_write_nand_cleanmarker(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);

diff -u ./mtd/fs/jffs2/read.c ./mtd_pierre/fs/jffs2/read.c
--- ./mtd/fs/jffs2/read.c       2005-07-22 19:32:08.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/read.c        2005-10-24 17:14:45.000000000 +0900
@@ -114,10 +114,8 @@
        ret = jffs2_flash_read(c, (ref_offset(fd->raw)) + sizeof(*ri),
                               je32_to_cpu(ri->csize), &readlen, readbuf);

-       if (!ret && readlen != je32_to_cpu(ri->csize))
-               ret = -EIO;
-       if (ret)
-               goto out_decomprbuf;
+       if (!jffs2_check_flash_read(ret, (ref_offset(fd->raw)) + sizeof(*ri), je32_to_cpu(ri->csize), readlen, (unsigned char*)&readbuf))
+               return ret;

        crc = crc32(0, readbuf, je32_to_cpu(ri->csize));
        if (crc != je32_to_cpu(ri->data_crc)) {

diff -u ./mtd/fs/jffs2/readinode.c ./mtd_pierre/fs/jffs2/readinode.c
--- ./mtd/fs/jffs2/readinode.c  2005-09-20 23:27:34.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/readinode.c   2005-10-24 17:15:11.000000000 +0900
@@ -427,17 +427,8 @@
        dbg_readinode("read more %d bytes\n", len);

        err = jffs2_flash_read(c, offs, len, &retlen, bufstart);
-       if (err) {
-               JFFS2_ERROR("can not read %d bytes from 0x%08x, "
-                       "error code: %d.\n", len, offs, err);
+       if (!jffs2_check_flash_read(err, offs, len, retlen, (unsigned char*)&bufstart))
                return err;
-       }
-
-       if (retlen < len) {
-               JFFS2_ERROR("short read at %#08x: %d instead of %d.\n",
-                               offs, retlen, len);
-               return -EIO;
-       }

        *rdlen = right_len;

diff -u ./mtd/fs/jffs2/scan.c ./mtd_pierre/fs/jffs2/scan.c
--- ./mtd/fs/jffs2/scan.c       2005-09-30 22:59:13.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/scan.c        2005-10-24 17:15:30.000000000 +0900
@@ -276,14 +276,8 @@
        size_t retlen;

        ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
-       if (ret) {
-               D1(printk(KERN_WARNING "mtd->read(0x%x bytes from 0x%x) returned %d\n", len, ofs, ret));
+       if (!jffs2_check_flash_read(ret, ofs, len, retlen, (void *)&buf))
                return ret;
-       }
-       if (retlen < len) {
-               D1(printk(KERN_WARNING "Read at 0x%x gave only 0x%zx bytes\n", ofs, retlen));
-               return -EIO;
-       }
        D2(printk(KERN_DEBUG "Read 0x%x bytes from 0x%08x into buf\n", len, ofs));
        D2(printk(KERN_DEBUG "000: %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
                  buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], buf[13], buf[14], buf[15]));

diff -u ./mtd/fs/jffs2/wbuf.c ./mtd_pierre/fs/jffs2/wbuf.c
--- ./mtd/fs/jffs2/wbuf.c       2005-09-30 22:59:13.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/wbuf.c        2005-10-24 19:03:02.000000000 +0900
@@ -923,6 +923,28 @@
 }

 /*
+ *     Check if jffs2_flash_read was successful
+ */
+int jffs2_check_flash_read(int err, uint32_t ofs, int len, size_t retlen, u_char *buf)
+{
+       int ret = 1;
+
+       /* did the read succeed? */
+       if (err) {
+               JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
+               kfree(buf);
+               ret = 0;
+       }
+       /* did we read all? */
+       if (retlen != len) {
+               JFFS2_ERROR("short read at 0x%08x: %d instead of %d.\n", ofs, retlen, len);
+               kfree(buf);
+               ret = 0;
+       }
+       return ret;
+}
+
+/*
  *     Check, if the out of band area is empty
  */
 int jffs2_check_oob_empty( struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, int mode)
@@ -946,17 +968,8 @@
         * to take care of the cleanmarker in the first page of the block
        */
        ret = jffs2_flash_read_oob(c, jeb->offset, len , &retlen, buf);
-       if (ret) {
-               D1(printk(KERN_WARNING "jffs2_check_oob_empty(): Read OOB failed %d for block at %08x\n", ret, jeb->offset));
-               goto out;
-       }
-
-       if (retlen < len) {
-               D1(printk(KERN_WARNING "jffs2_check_oob_empty(): Read OOB return short read "
-                         "(%zd bytes not %d) for block at %08x\n", retlen, len, jeb->offset));
-               ret = -EIO;
-               goto out;
-       }
+       if (!jffs2_check_flash_read(ret, jeb->offset, len, retlen, (unsigned char*)&buf))
+               return ret;

        /* Special check for first page */
        for(i = 0; i < oob_size ; i++) {

Re: [PATCH] separate routine to check jffs2_flash_read

[Jörn Engel]

On Wed, 26 October 2005 09:32:44 +0200, Pierre.Ricadat@UTBM.fr wrote:
> 
> here i attach a small patch for that 'todo' in nodelist.c:
> 
>   /* TODO: this is very frequent pattern, make it a separate
> 		   * routine */
>   err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
>   ...
> 
> about the checking of jffs2_flash_read().
> 
> I made a new function : jffs2_check_flash_read to
> - check that jffs2_flash_read returns success
> - check that it read all
> and in a case of failure, print error, free the buffer and return error code.

Nice.  Two things you could improve:

> @@ -131,12 +131,8 @@
>                 return;
> 
>         ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
> -       if (ret || (retlen != len)) {
> -               JFFS2_WARNING("read %d bytes failed or short. ret %d, retlen %zd.\n",
> -                               len, ret, retlen);
> -               kfree(buf);
> +       if (!jffs2_check_flash_read(ret, ofs, len, retlen, &buf))
>                 return;
> -       }

All cases but one call jffs2_flash_read() before doing the various
error checks.  A function jffs2_flash_read_safe() that combines your
existing function and the actual read would be even nicer.


>  /*
> + *     Check if jffs2_flash_read was successful
> + */
> +int jffs2_check_flash_read(int err, uint32_t ofs, int len, size_t retlen, u_char *buf)
> +{
> +       int ret = 1;
> +
> +       /* did the read succeed? */
> +       if (err) {
> +               JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
> +               kfree(buf);
> +               ret = 0;
> +       }
> +       /* did we read all? */
> +       if (retlen != len) {
> +               JFFS2_ERROR("short read at 0x%08x: %d instead of %d.\n", ofs, retlen, len);
> +               kfree(buf);
> +               ret = 0;
> +       }
> +       return ret;
> +}

The return code doesn't match common convention at all - people will
get confused and create bugs in the future.  Return 0 on success and
-EIO for errors.


Jörn

-- 
Schrödinger's cat is <BLINK>not</BLINK> dead.
-- Illiad

Re: [PATCH] separate routine to check jffs2_flash_read

[pierre.ricadat@utbm.fr]

[-- Attachment #1: Type: text/plain, Size: 199 bytes --]

Hi'

I made the two changes you proposed:
- new function (jffs2_flash_read_safe) who calls jffs2_flash_read and checks it
- better return code

So i attach the new patch.

Regards

-- 
Pierre Ricadat

[-- Attachment #2: jffs2_flash_read_safe.patch --]
[-- Type: application/octet-stream, Size: 11386 bytes --]

diff -u ./mtd/fs/jffs2/debug.c ./mtd_pierre/fs/jffs2/debug.c
--- ./mtd/fs/jffs2/debug.c      2005-09-21 22:28:35.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/debug.c       2005-10-31 18:40:01.000000000 +0900
@@ -130,13 +130,8 @@
        if (!buf)
                return;

-       ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
-       if (ret || (retlen != len)) {
-               JFFS2_WARNING("read %d bytes failed or short. ret %d, retlen %zd.\n",
-                               len, ret, retlen);
-               kfree(buf);
+       if (jffs2_flash_read_safe(c, ofs, len, buf))
                return;
-       }

        ret = 0;
        for (i = 0; i < len; i++)
@@ -615,16 +610,11 @@
        int len = sizeof(union jffs2_node_union);
        size_t retlen;
        uint32_t crc;
-       int ret;

        printk(JFFS2_DBG_MSG_PREFIX " dump node at offset %#08x.\n", ofs);

-       ret = jffs2_flash_read(c, ofs, len, &retlen, (unsigned char *)&node);
-       if (ret || (retlen != len)) {
-               JFFS2_ERROR("read %d bytes failed or short. ret %d, retlen %zd.\n",
-                       len, ret, retlen);
+       if (jffs2_flash_read_safe(c, ofs, len, (unsigned char *)&node))
                return;
-       }

        printk(JFFS2_DBG "magic:\t%#04x\n", je16_to_cpu(node.u.magic));
        printk(JFFS2_DBG "nodetype:\t%#04x\n", je16_to_cpu(node.u.nodetype));

diff -u ./mtd/fs/jffs2/erase.c ./mtd_pierre/fs/jffs2/erase.c
--- ./mtd/fs/jffs2/erase.c      2005-09-20 23:53:15.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/erase.c       2005-10-31 18:25:29.000000000 +0900
@@ -305,7 +305,6 @@
 {
        void *ebuf;
        uint32_t ofs;
-       size_t retlen;
        int ret = -EIO;

        ebuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
@@ -322,15 +321,8 @@

                *bad_offset = ofs;

-               ret = jffs2_flash_read(c, ofs, readlen, &retlen, ebuf);
-               if (ret) {
-                       printk(KERN_WARNING "Read of newly-erased block at 0x%08x failed: %d. Putting on bad_list\n", ofs, ret);
-                       goto fail;
-               }
-               if (retlen != readlen) {
-                       printk(KERN_WARNING "Short read from newly-erased block at 0x%08x. Wanted %d, got %zd\n", ofs, readlen, retlen);
-                       goto fail;
-               }
+               if (jffs2_flash_read_safe(c, ofs, readlen, (void *)ebuf))
+                       return ret;
                for (i=0; i<readlen; i += sizeof(unsigned long)) {
                        /* It's OK. We know it's properly aligned */
                        unsigned long *datum = ebuf + i;

diff -u ./mtd/fs/jffs2/gc.c ./mtd_pierre/fs/jffs2/gc.c
--- ./mtd/fs/jffs2/gc.c 2005-09-07 17:34:54.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/gc.c  2005-10-31 18:34:41.000000000 +0900
@@ -530,11 +530,8 @@
        if (!node)
                return -ENOMEM;

-       ret = jffs2_flash_read(c, ref_offset(raw), rawlen, &retlen, (char *)node);
-       if (!ret && retlen != rawlen)
-               ret = -EIO;
-       if (ret)
-               goto out_node;
+       if (jffs2_flash_read_safe(c, ref_offset(raw), rawlen, (char*)node))
+               return -EIO;

        crc = crc32(0, node, sizeof(struct jffs2_unknown_node)-4);
        if (je32_to_cpu(node->u.hdr_crc) != crc) {
@@ -818,8 +815,7 @@
        if (!jffs2_can_mark_obsolete(c)) {
                struct jffs2_raw_dirent *rd;
                struct jffs2_raw_node_ref *raw;
-               int ret;
-               size_t retlen;
+
                int name_len = strlen(fd->name);
                uint32_t name_crc = crc32(0, fd->name, name_len);
                uint32_t rawlen = ref_totlen(c, jeb, fd->raw);
@@ -852,17 +848,9 @@

                        /* This is an obsolete node belonging to the same directory, and it's of the right
                           length. We need to take a closer look...*/
-                       ret = jffs2_flash_read(c, ref_offset(raw), rawlen, &retlen, (char *)rd);
-                       if (ret) {
-                               printk(KERN_WARNING "jffs2_g_c_deletion_dirent(): Read error (%d) reading obsolete node at %08x\n", ret, ref_offset(raw));
+                       if (jffs2_flash_read_safe(c, ref_offset(raw), rawlen, (char *)rd))
                                /* If we can't read it, we don't need to continue to obsolete it. Continue */
                                continue;
-                       }
-                       if (retlen != rawlen) {
-                               printk(KERN_WARNING "jffs2_g_c_deletion_dirent(): Short read (%zd not %u) reading header from obsolete node at %08x\n",
-                                      retlen, rawlen, ref_offset(raw));
-                               continue;
-                       }

                        if (je16_to_cpu(rd->nodetype) != JFFS2_NODETYPE_DIRENT)
                                continue;

diff -u ./mtd/fs/jffs2/nodelist.c ./mtd_pierre/fs/jffs2/nodelist.c
--- ./mtd/fs/jffs2/nodelist.c   2005-09-21 22:28:35.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/nodelist.c    2005-10-31 18:27:34.000000000 +0900
@@ -451,19 +451,8 @@
                if (unlikely(!buffer))
                        return -ENOMEM;

-               /* TODO: this is very frequent pattern, make it a separate
-                * routine */
-               err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
-               if (err) {
-                       JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
-                       goto free_out;
-               }
-
-               if (retlen != len) {
-                       JFFS2_ERROR("short read at %#08x: %d instead of %d.\n", ofs, retlen, len);
-                       err = -EIO;
-                       goto free_out;
-               }
+               if (jffs2_flash_read_safe(c, ofs, len, buffer))
+                       return -EIO;
        }

        /* Continue calculating CRC */
@@ -498,14 +487,6 @@

        return 0;

-free_out:
-       if(!pointed)
-               kfree(buffer);
-#ifndef __ECOS
-       else
-               c->mtd->unpoint(c->mtd, buffer, ofs, len);
-#endif
-       return err;
 }

diff -u ./mtd/fs/jffs2/os-linux.h ./mtd_pierre/fs/jffs2/os-linux.h
--- ./mtd/fs/jffs2/os-linux.h   2005-09-30 22:59:13.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/os-linux.h    2005-10-31 17:04:13.000000000 +0900
@@ -122,6 +122,7 @@
 int jffs2_flash_writev(struct jffs2_sb_info *c, const struct kvec *vecs, unsigned long count, loff_t to, size_t *retlen, uint32_t ino);
 int jffs2_flash_write(struct jffs2_sb_info *c, loff_t ofs, size_t len, size_t *retlen, const u_char *buf);
 int jffs2_flash_read(struct jffs2_sb_info *c, loff_t ofs, size_t len, size_t *retlen, u_char *buf);
+int jffs2_flash_read_safe(struct jffs2_sb_info *c, uint32_t ofs, int len, u_char *buf);
 int jffs2_check_oob_empty(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,int mode);
 int jffs2_check_nand_cleanmarker(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
 int jffs2_write_nand_cleanmarker(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);

diff -u ./mtd/fs/jffs2/read.c ./mtd_pierre/fs/jffs2/read.c
--- ./mtd/fs/jffs2/read.c       2005-07-22 19:32:08.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/read.c        2005-10-31 18:28:16.000000000 +0900
@@ -111,13 +111,9 @@

        D2(printk(KERN_DEBUG "Read %d bytes to %p\n", je32_to_cpu(ri->csize),
                  readbuf));
-       ret = jffs2_flash_read(c, (ref_offset(fd->raw)) + sizeof(*ri),
-                              je32_to_cpu(ri->csize), &readlen, readbuf);
-
-       if (!ret && readlen != je32_to_cpu(ri->csize))
-               ret = -EIO;
-       if (ret)
-               goto out_decomprbuf;
+
+       if (jffs2_flash_read_safe(c, (ref_offset(fd->raw)) + sizeof(*ri), je32_to_cpu(ri->csize), readbuf))
+               return -EIO;

        crc = crc32(0, readbuf, je32_to_cpu(ri->csize));
        if (crc != je32_to_cpu(ri->data_crc)) {

diff -u ./mtd/fs/jffs2/readinode.c ./mtd_pierre/fs/jffs2/readinode.c
--- ./mtd/fs/jffs2/readinode.c  2005-09-20 23:27:34.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/readinode.c   2005-10-31 18:28:33.000000000 +0900
@@ -400,8 +400,7 @@
 static int read_more(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *ref,
                     int right_size, int *rdlen, unsigned char *buf, unsigned char *bufstart)
 {
-       int right_len, err, len;
-       size_t retlen;
+       int right_len, len;
        uint32_t offs;

        if (jffs2_is_writebuffered(c)) {
@@ -426,18 +425,8 @@

        dbg_readinode("read more %d bytes\n", len);

-       err = jffs2_flash_read(c, offs, len, &retlen, bufstart);
-       if (err) {
-               JFFS2_ERROR("can not read %d bytes from 0x%08x, "
-                       "error code: %d.\n", len, offs, err);
-               return err;
-       }
-
-       if (retlen < len) {
-               JFFS2_ERROR("short read at %#08x: %d instead of %d.\n",
-                               offs, retlen, len);
+       if (jffs2_flash_read_safe(c, offs, len, bufstart))
                return -EIO;
-       }

        *rdlen = right_len;

diff -u ./mtd/fs/jffs2/scan.c ./mtd_pierre/fs/jffs2/scan.c
--- ./mtd/fs/jffs2/scan.c       2005-09-30 22:59:13.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/scan.c        2005-10-31 18:29:23.000000000 +0900
@@ -272,18 +272,9 @@
 int jffs2_fill_scan_buf (struct jffs2_sb_info *c, void *buf,
                                uint32_t ofs, uint32_t len)
 {
-       int ret;
-       size_t retlen;

-       ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
-       if (ret) {
-               D1(printk(KERN_WARNING "mtd->read(0x%x bytes from 0x%x) returned %d\n", len, ofs, ret));
-               return ret;
-       }
-       if (retlen < len) {
-               D1(printk(KERN_WARNING "Read at 0x%x gave only 0x%zx bytes\n", ofs, retlen));
+       if (jffs2_flash_read_safe(c, ofs, len, buf))
                return -EIO;
-       }
        D2(printk(KERN_DEBUG "Read 0x%x bytes from 0x%08x into buf\n", len, ofs));
        D2(printk(KERN_DEBUG "000: %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
                  buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], buf[13], buf[14], buf[15]));

diff -u ./mtd/fs/jffs2/wbuf.c ./mtd_pierre/fs/jffs2/wbuf.c
--- ./mtd/fs/jffs2/wbuf.c       2005-09-30 22:59:13.000000000 +0900
+++ ./mtd_pierre/fs/jffs2/wbuf.c        2005-10-31 17:25:36.000000000 +0900
@@ -923,6 +923,32 @@
 }

 /*
+ *     Check if jffs2_flash_read was successful
+ */
+int jffs2_flash_read_safe(struct jffs2_sb_info *c, uint32_t ofs, int len, u_char *buf)
+{
+        size_t retlen;
+       int err, ret = 0;
+
+       /* read the data */
+       err = jffs2_flash_read(c, ofs, len, &retlen, buf);
+
+       /* did the read succeed? */
+       if (err) {
+               JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
+               kfree(buf);
+               ret = -EIO;
+       }
+       /* did we read all? */
+       if (retlen != len) {
+               JFFS2_ERROR("short read at 0x%08x: %d instead of %d.\n", ofs, retlen, len);
+               kfree(buf);
+               ret = -EIO;
+       }
+       return ret;
+}
+
+/*
  *     Check, if the out of band area is empty
  */

Re: [PATCH] separate routine to check jffs2_flash_read

[Jörn Engel]

On Mon, 31 October 2005 11:12:06 +0100, pierre.ricadat@utbm.fr wrote:
> 
> I made the two changes you proposed:
> - new function (jffs2_flash_read_safe) who calls jffs2_flash_read and checks it
> - better return code
> 
> So i attach the new patch.

Better.  There's one bit that actually concerns me, though:

> diff -u ./mtd/fs/jffs2/nodelist.c ./mtd_pierre/fs/jffs2/nodelist.c
> --- ./mtd/fs/jffs2/nodelist.c   2005-09-21 22:28:35.000000000 +0900
> +++ ./mtd_pierre/fs/jffs2/nodelist.c    2005-10-31 18:27:34.000000000 +0900
> @@ -451,19 +451,8 @@
>                 if (unlikely(!buffer))
>                         return -ENOMEM;
> 
> -               /* TODO: this is very frequent pattern, make it a separate
> -                * routine */
> -               err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
> -               if (err) {
> -                       JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
> -                       goto free_out;
> -               }
> -
> -               if (retlen != len) {
> -                       JFFS2_ERROR("short read at %#08x: %d instead of %d.\n", ofs, retlen, len);
> -                       err = -EIO;
> -                       goto free_out;
> -               }
> +               if (jffs2_flash_read_safe(c, ofs, len, buffer))
> +                       return -EIO;
>         }
> 
>         /* Continue calculating CRC */
> @@ -498,14 +487,6 @@
> 
>         return 0;
> 
> -free_out:
> -       if(!pointed)
> -               kfree(buffer);
> -#ifndef __ECOS
> -       else
> -               c->mtd->unpoint(c->mtd, buffer, ofs, len);
> -#endif
> -       return err;
>  }

Nowhere in jffs2_flash_read_safe() do you call c->mtd->unpoint.  So
obviously your code is not equivalent to the existing.  Can you look
into it and see what needs to be don?


Jörn

-- 
When you close your hand, you own nothing. When you open it up, you
own the whole world.
-- Li Mu Bai in Tiger & Dragon

Re: [PATCH] separate routine to check jffs2_flash_read

[pierre.ricadat@utbm.fr]

Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:

> On Mon, 31 October 2005 11:12:06 +0100, pierre.ricadat@utbm.fr wrote:
> >
> > -free_out:
> > -       if(!pointed)
> > -               kfree(buffer);
> > -#ifndef __ECOS
> > -       else
> > -               c->mtd->unpoint(c->mtd, buffer, ofs, len);
> > -#endif
> > -       return err;
> >  }
>
> Nowhere in jffs2_flash_read_safe() do you call c->mtd->unpoint.  So
> obviously your code is not equivalent to the existing.  Can you look
> into it and see what needs to be don?

In fact the only place where we goto free_out was there :

@ line 449:
	if (!pointed) {
		buffer = kmalloc(len, GFP_KERNEL);
		if (unlikely(!buffer))
			return -ENOMEM;

		/* TODO: this is very frequent pattern, make it a separate
		 * routine */
		err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
		if (err) {
			JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs,
err);
			goto free_out;
		}

		if (retlen != len) {
			JFFS2_ERROR("short read at %#08x: %d instead of %d.\n", ofs, retlen, len);
			err = -EIO;
			goto free_out;
		}
	}

The goto free_out was located in a if(!pointed), so the if(!pointed) in the
free_out will always be true, and c->mtd->unpoint will never be called.

-- 
Pierre Ricadat

Re: [PATCH] separate routine to check jffs2_flash_read

[Jörn Engel]

On Mon, 31 October 2005 16:16:17 +0100, pierre.ricadat@utbm.fr wrote:
> Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:
> > On Mon, 31 October 2005 11:12:06 +0100, pierre.ricadat@utbm.fr wrote:
> > >
> > > -free_out:
> > > -       if(!pointed)
> > > -               kfree(buffer);
> > > -#ifndef __ECOS
> > > -       else
> > > -               c->mtd->unpoint(c->mtd, buffer, ofs, len);
> > > -#endif
> > > -       return err;
> > >  }
> >
> > Nowhere in jffs2_flash_read_safe() do you call c->mtd->unpoint.  So
> > obviously your code is not equivalent to the existing.  Can you look
> > into it and see what needs to be don?
> 
> In fact the only place where we goto free_out was there :
> 
> @ line 449:
> 	if (!pointed) {
> 		buffer = kmalloc(len, GFP_KERNEL);
> 		if (unlikely(!buffer))
> 			return -ENOMEM;
> 
> 		/* TODO: this is very frequent pattern, make it a separate
> 		 * routine */
> 		err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
> 		if (err) {
> 			JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs,
> err);
> 			goto free_out;
> 		}
> 
> 		if (retlen != len) {
> 			JFFS2_ERROR("short read at %#08x: %d instead of %d.\n", ofs, retlen, len);
> 			err = -EIO;
> 			goto free_out;
> 		}
> 	}
> 
> The goto free_out was located in a if(!pointed), so the if(!pointed) in the
> free_out will always be true, and c->mtd->unpoint will never be called.

Hmmyess, makes sense.  I'd vote for inclusion "right after the merge".

Not sure what that actually means.  If it takes too long, I might just
shrug and dump new stuff into cvs.  We've had a calm-down period for a
while now and the merge could even happen with code from some date
like today.

Jörn

-- 
The competent programmer is fully aware of the strictly limited size of
his own skull; therefore he approaches the programming task in full
humility, and among other things he avoids clever tricks like the plague. 
-- Edsger W. Dijkstra

Re: [PATCH] separate routine to check jffs2_flash_read

[Pierre.Ricadat@UTBM.fr]

Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:

> Hmmyess, makes sense.  I'd vote for inclusion "right after the merge".
>
> Not sure what that actually means.  If it takes too long, I might just
> shrug and dump new stuff into cvs.  We've had a calm-down period for a
> while now and the merge could even happen with code from some date
> like today.
>
> Jörn

Could it be added to cvs now?

Regards
-- 
Pierre Ricadat

Re: [PATCH] separate routine to check jffs2_flash_read

[Jörn Engel]

On Fri, 11 November 2005 10:44:19 +0100, Pierre.Ricadat@UTBM.fr wrote:
> Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:
> 
> > Hmmyess, makes sense.  I'd vote for inclusion "right after the merge".
> >
> > Not sure what that actually means.  If it takes too long, I might just
> > shrug and dump new stuff into cvs.  We've had a calm-down period for a
> > while now and the merge could even happen with code from some date
> > like today.
> 
> Could it be added to cvs now?

In principle, yes.  I get half a page of rejects, though.  Can you
respin the patch against current cvs, preferrably so that it applies
with patch -p1?

Jörn

-- 
Invincibility is in oneself, vulnerability is in the opponent.
-- Sun Tzu

Re: [PATCH] separate routine to check jffs2_flash_read

[Jörn Engel]

On Fri, 11 November 2005 12:14:02 +0100, Pierre.Ricadat@UTBM.fr wrote:
> Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:
> > In principle, yes.  I get half a page of rejects, though.  Can you
> > respin the patch against current cvs, preferrably so that it applies
> > with patch -p1?
> 
> Here is the new patch for current cvs.

Unfortunately in DOS format (0x13,0x10 line breaks).  Can you respin
it into Unix format?

Jörn

-- 
When in doubt, use brute force.
-- Ken Thompson

Re: [PATCH] separate routine to check jffs2_flash_read

[Pierre.Ricadat@UTBM.fr]

[-- Attachment #1: Type: text/plain, Size: 243 bytes --]

Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:
> > Here is the new patch for current cvs.
>
> Unfortunately in DOS format (0x13,0x10 line breaks).  Can you respin
> it into Unix format?

Oops. Sorry. This is the good one.

-- 
Pierre Ricadat

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: jffs2_flash_read_safe.patch --]
[-- Type: text/x-patch; name="jffs2_flash_read_safe.patch", Size: 11387 bytes --]

diff -u mtd/fs/jffs2/debug.c mtd_pierre/fs/jffs2/debug.c
--- mtd/fs/jffs2/debug.c      2005-11-08 15:56:34.000000000 +0900
+++ mtd_pierre/fs/jffs2/debug.c       2005-11-11 19:41:43.000000000 +0900
@@ -130,13 +130,8 @@
        if (!buf)
                return;

-       ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
-       if (ret || (retlen != len)) {
-               JFFS2_WARNING("read %d bytes failed or short. ret %d, retlen %zd.\n",
-                               len, ret, retlen);
-               kfree(buf);
+       if (jffs2_flash_read_safe(c, ofs, len, buf))
                return;
-       }

        ret = 0;
        for (i = 0; i < len; i++)
@@ -615,16 +610,11 @@
        int len = sizeof(union jffs2_node_union);
        size_t retlen;
        uint32_t crc;
-       int ret;

        printk(JFFS2_DBG_MSG_PREFIX " dump node at offset %#08x.\n", ofs);

-       ret = jffs2_flash_read(c, ofs, len, &retlen, (unsigned char *)&node);
-       if (ret || (retlen != len)) {
-               JFFS2_ERROR("read %d bytes failed or short. ret %d, retlen %zd.\n",
-                       len, ret, retlen);
+       if (jffs2_flash_read_safe(c, ofs, len, (unsigned char *)&node))
                return;
-       }

        printk(JFFS2_DBG "magic:\t%#04x\n", je16_to_cpu(node.u.magic));
        printk(JFFS2_DBG "nodetype:\t%#04x\n", je16_to_cpu(node.u.nodetype));

diff -u mtd/fs/jffs2/erase.c mtd_pierre/fs/jffs2/erase.c
--- mtd/fs/jffs2/erase.c      2005-11-08 15:56:37.000000000 +0900
+++ mtd_pierre/fs/jffs2/erase.c       2005-11-11 19:43:21.000000000 +0900
@@ -306,7 +306,6 @@
 {
        void *ebuf;
        uint32_t ofs;
-       size_t retlen;
        int ret = -EIO;

        ebuf = kmalloc(PAGE_SIZE, GFP_KERNEL);
@@ -323,15 +322,8 @@

                *bad_offset = ofs;

-               ret = jffs2_flash_read(c, ofs, readlen, &retlen, ebuf);
-               if (ret) {
-                       printk(KERN_WARNING "Read of newly-erased block at 0x%08x failed: %d. Putting on bad_list\n", ofs, ret);
-                       goto fail;
-               }
-               if (retlen != readlen) {
-                       printk(KERN_WARNING "Short read from newly-erased block at 0x%08x. Wanted %d, got %zd\n", ofs, readlen, retlen);
-                       goto fail;
-               }
+               if (jffs2_flash_read_safe(c, ofs, readlen, (void *)ebuf))
+                       return ret;
                for (i=0; i<readlen; i += sizeof(unsigned long)) {
                        /* It's OK. We know it's properly aligned */
                        unsigned long *datum = ebuf + i;

diff -u mtd/fs/jffs2/gc.c mtd_pierre/fs/jffs2/gc.c
--- mtd/fs/jffs2/gc.c 2005-11-11 19:35:06.000000000 +0900
+++ mtd_pierre/fs/jffs2/gc.c  2005-11-11 19:45:43.000000000 +0900
@@ -530,11 +530,8 @@
        if (!node)
                return -ENOMEM;

-       ret = jffs2_flash_read(c, ref_offset(raw), rawlen, &retlen, (char *)node);
-       if (!ret && retlen != rawlen)
-               ret = -EIO;
-       if (ret)
-               goto out_node;
+       if (jffs2_flash_read_safe(c, ref_offset(raw), rawlen, (char*)node))
+               return -EIO;

        crc = crc32(0, node, sizeof(struct jffs2_unknown_node)-4);
        if (je32_to_cpu(node->u.hdr_crc) != crc) {
@@ -818,8 +815,6 @@
        if (!jffs2_can_mark_obsolete(c)) {
                struct jffs2_raw_dirent *rd;
                struct jffs2_raw_node_ref *raw;
-               int ret;
-               size_t retlen;
                int name_len = strlen(fd->name);
                uint32_t name_crc = crc32(0, fd->name, name_len);
                uint32_t rawlen = ref_totlen(c, jeb, fd->raw);
@@ -852,17 +847,9 @@

                        /* This is an obsolete node belonging to the same directory, and it's of the right
                           length. We need to take a closer look...*/
-                       ret = jffs2_flash_read(c, ref_offset(raw), rawlen, &retlen, (char *)rd);
-                       if (ret) {
-                               printk(KERN_WARNING "jffs2_g_c_deletion_dirent(): Read error (%d) reading obsolete node at %08x\n", ret, ref_offset(raw));
+                       if (jffs2_flash_read_safe(c, ref_offset(raw), rawlen, (char *)rd))
                                /* If we can't read it, we don't need to continue to obsolete it. Continue */
                                continue;
-                       }
-                       if (retlen != rawlen) {
-                               printk(KERN_WARNING "jffs2_g_c_deletion_dirent(): Short read (%zd not %u) reading header from obsolete node at %08x\n",
-                                      retlen, rawlen, ref_offset(raw));
-                               continue;
-                       }

                        if (je16_to_cpu(rd->nodetype) != JFFS2_NODETYPE_DIRENT)
                                continue;

diff -u mtd/fs/jffs2/nodelist.c mtd_pierre/fs/jffs2/nodelist.c
--- mtd/fs/jffs2/nodelist.c   2005-11-11 19:35:08.000000000 +0900
+++ mtd_pierre/fs/jffs2/nodelist.c    2005-11-11 19:47:27.000000000 +0900
@@ -451,19 +451,8 @@
                if (unlikely(!buffer))
                        return -ENOMEM;

-               /* TODO: this is very frequent pattern, make it a separate
-                * routine */
-               err = jffs2_flash_read(c, ofs, len, &retlen, buffer);
-               if (err) {
-                       JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
-                       goto free_out;
-               }
-
-               if (retlen != len) {
-                       JFFS2_ERROR("short read at %#08x: %d instead of %d.\n", ofs, retlen, len);
-                       err = -EIO;
-                       goto free_out;
-               }
+               if (jffs2_flash_read_safe(c, ofs, len, buffer))
+                       return -EIO;
        }

        /* Continue calculating CRC */
@@ -497,15 +486,6 @@
        spin_unlock(&c->erase_completion_lock);

        return 0;
-
-free_out:
-       if(!pointed)
-               kfree(buffer);
-#ifndef __ECOS
-       else
-               c->mtd->unpoint(c->mtd, buffer, ofs, len);
-#endif
-       return err;
 }

 /*

diff -u mtd/fs/jffs2/os-linux.h mtd_pierre/fs/jffs2/os-linux.h
--- mtd/fs/jffs2/os-linux.h   2005-11-08 15:56:52.000000000 +0900
+++ mtd_pierre/fs/jffs2/os-linux.h    2005-11-11 19:48:02.000000000 +0900
@@ -122,6 +122,7 @@
 int jffs2_flash_writev(struct jffs2_sb_info *c, const struct kvec *vecs, unsigned long count, loff_t to, size_t *retlen, uint32_t ino);
 int jffs2_flash_write(struct jffs2_sb_info *c, loff_t ofs, size_t len, size_t *retlen, const u_char *buf);
 int jffs2_flash_read(struct jffs2_sb_info *c, loff_t ofs, size_t len, size_t *retlen, u_char *buf);
+int jffs2_flash_read_safe(struct jffs2_sb_info *c, uint32_t ofs, int len, u_char *buf);
 int jffs2_check_oob_empty(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,uint32_t data_len);
 int jffs2_check_nand_cleanmarker_ebh(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t *data_len);
 int jffs2_write_nand_ebh(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);

diff -u mtd/fs/jffs2/read.c mtd_pierre/fs/jffs2/read.c
--- mtd/fs/jffs2/read.c       2005-11-08 15:56:54.000000000 +0900
+++ mtd_pierre/fs/jffs2/read.c        2005-11-11 19:49:02.000000000 +0900
@@ -111,13 +111,9 @@

        D2(printk(KERN_DEBUG "Read %d bytes to %p\n", je32_to_cpu(ri->csize),
                  readbuf));
-       ret = jffs2_flash_read(c, (ref_offset(fd->raw)) + sizeof(*ri),
-                              je32_to_cpu(ri->csize), &readlen, readbuf);

-       if (!ret && readlen != je32_to_cpu(ri->csize))
-               ret = -EIO;
-       if (ret)
-               goto out_decomprbuf;
+       if (jffs2_flash_read_safe(c, (ref_offset(fd->raw)) + sizeof(*ri), je32_to_cpu(ri->csize), readbuf))
+               return -EIO;

        crc = crc32(0, readbuf, je32_to_cpu(ri->csize));
        if (crc != je32_to_cpu(ri->data_crc)) {

diff -u mtd/fs/jffs2/readinode.c mtd_pierre/fs/jffs2/readinode.c
--- mtd/fs/jffs2/readinode.c  2005-11-11 19:35:13.000000000 +0900
+++ mtd_pierre/fs/jffs2/readinode.c   2005-11-11 19:50:19.000000000 +0900
@@ -400,8 +400,7 @@
 static int read_more(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *ref,
                     int right_size, int *rdlen, unsigned char *buf, unsigned char *bufstart)
 {
-       int right_len, err, len;
-       size_t retlen;
+       int right_len, len;
        uint32_t offs;

        if (jffs2_is_writebuffered(c)) {
@@ -426,18 +425,8 @@

        dbg_readinode("read more %d bytes\n", len);

-       err = jffs2_flash_read(c, offs, len, &retlen, bufstart);
-       if (err) {
-               JFFS2_ERROR("can not read %d bytes from 0x%08x, "
-                       "error code: %d.\n", len, offs, err);
-               return err;
-       }
-
-       if (retlen < len) {
-               JFFS2_ERROR("short read at %#08x: %d instead of %d.\n",
-                               offs, retlen, len);
+       if (jffs2_flash_read_safe(c, offs, len, bufstart))
                return -EIO;
-       }

        *rdlen = right_len;

diff -u mtd/fs/jffs2/scan.c mtd_pierre/fs/jffs2/scan.c
--- mtd/fs/jffs2/scan.c       2005-11-11 19:35:17.000000000 +0900
+++ mtd_pierre/fs/jffs2/scan.c        2005-11-11 19:52:06.000000000 +0900
@@ -274,18 +274,8 @@
 int jffs2_fill_scan_buf (struct jffs2_sb_info *c, void *buf,
                                uint32_t ofs, uint32_t len)
 {
-       int ret;
-       size_t retlen;
-
-       ret = jffs2_flash_read(c, ofs, len, &retlen, buf);
-       if (ret) {
-               D1(printk(KERN_WARNING "mtd->read(0x%x bytes from 0x%x) returned %d\n", len, ofs, ret));
-               return ret;
-       }
-       if (retlen < len) {
-               D1(printk(KERN_WARNING "Read at 0x%x gave only 0x%zx bytes\n", ofs, retlen));
+        if (jffs2_flash_read_safe(c, ofs, len, buf))
                return -EIO;
-       }
        D2(printk(KERN_DEBUG "Read 0x%x bytes from 0x%08x into buf\n", len, ofs));
        D2(printk(KERN_DEBUG "000: %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x %02x\n",
                  buf[0], buf[1], buf[2], buf[3], buf[4], buf[5], buf[6], buf[7], buf[8], buf[9], buf[10], buf[11], buf[12], buf[13], buf[14], buf[15]));

diff -u mtd/fs/jffs2/wbuf.c mtd_pierre/fs/jffs2/wbuf.c
--- mtd/fs/jffs2/wbuf.c       2005-11-11 19:35:21.000000000 +0900
+++ mtd_pierre/fs/jffs2/wbuf.c        2005-11-11 19:53:27.000000000 +0900
@@ -925,6 +925,32 @@
 }

 /*
+ *     Check if jffs2_flash_read was successful
+ */
+int jffs2_flash_read_safe(struct jffs2_sb_info *c, uint32_t ofs, int len, u_char *buf)
+{
+       size_t retlen;
+       int err, ret = 0;
+
+       /* read the data */
+       err = jffs2_flash_read(c, ofs, len, &retlen, buf);
+
+       /* did the read succeed? */
+       if (err) {
+               JFFS2_ERROR("can not read %d bytes from 0x%08x, error code: %d.\n", len, ofs, err);
+               kfree(buf);
+               ret = -EIO;
+       }
+       /* did we read all? */
+       if (retlen != len) {
+               JFFS2_ERROR("short read at 0x%08x: %d instead of %d.\n", ofs, retlen, len);
+               kfree(buf);
+               ret = -EIO;
+       }
+       return ret;
+}
+
+/*
  *     Check, if the out of band area is empty
  */

Re: [PATCH] separate routine to check jffs2_flash_read

[Thomas Gleixner]

On Fri, 2005-11-11 at 13:25 +0100, Pierre.Ricadat@UTBM.fr wrote:
> Quoting Jörn Engel <joern@wohnheim.fh-wedel.de>:
> > > Here is the new patch for current cvs.
> >
> > Unfortunately in DOS format (0x13,0x10 line breaks).  Can you respin
> > it into Unix format?
> 
> Oops. Sorry. This is the good one.

Good ? As long as we restrict the view to the file format.

The patch introduces:

- memory leaks
- use after free
- kfree of pointers pointing to a variable on the stack

Have a close look at all callers of this function.

In general, hiding kfree(var) in the error path of a global function,
which purpose is to read data from flash and handle the error conditions
in terms of messages and return value, is a secure source for above
problems. 

When neither the author himself nor a reviewer recognizes the hidden
trouble, how is an innocent user supposed not to trap into this ?

Unfortunately the patch was applied already. Fixed in CVS.

BTW, can we please start to add DocBook comments to new functions or to
functions which are reworked ? That way the documentation of the global
functions might be reality some day.


	tglx