Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

Jörn Engel
2005-07-22T11:59:41Z

On Thu, 21 July 2005 15:22:24 +0800, ?? ???? wrote:
>
> Current jffs2_check_oob_empty() assumes that only OOB of first
> page has data content -- the clean marker. OOB of other pages should
> be all 0xFF.
> But if we decide to store other kind of meta-data in OOB of the
> second,third or forth page, this will cause compatibility issue.
> For example, if we store some data in OOB of second page in the
> future, the "old" JFFS2 will recognize all the erase blocks in
> the "new" JFFS2 image as ALL_DIRTY.
> So this patch solve this compatibility problem by extending existent
> node type compat_bits to OOB area. In particular, I think
> JFFS2_FEATURE_RWCOMPAT_COPY has no meaning for OOB, so it's
> handled the same way as JFFS2_FEATURE_RWCOMPAT_DELETE.
>
> Your comments are welcome!

I'm pretty clueless wrt. oob data, so I leave the hard stuff for
others to comment on.

> --- wbuf.c.bak 2005-07-21 20:50:42.000000000 +0800
> +++ wbuf.c 2005-07-21 22:03:53.589883720 +0800
> @@ -933,9 +933,10 @@
> {
> unsigned char *buf;
> int ret = 0;
> - int i,len,page;
> + int i,len,page,current_oob;
> size_t retlen;
> int oob_size;
> + struct jffs2_unknown_node *node;
>
> /* allocate a buffer for all oob data in this sector */
> oob_size = c->mtd->oobsize;
> @@ -947,7 +948,7 @@
> }
> /*
> * if mode = 0, we scan for a total empty oob area, else we have
> - * to take care of the cleanmarker in the first page of the block
> + * to take care of the data content in the subsequent pages of the block
> */
> ret = jffs2_flash_read_oob(c, jeb->offset, len , &retlen, buf);
> if (ret) {
> @@ -961,28 +962,51 @@
> ret = -EIO;
> goto out;
> }
> -
> - /* Special check for first page */
> - for(i = 0; i < oob_size ; i++) {
> - /* Yeah, we know about the cleanmarker. */
> - if (mode && i >= c->fsdata_pos &&
> - i < c->fsdata_pos + c->fsdata_len)
> - continue;
> -
> - if (buf[i] != 0xFF) {
> - D2(printk(KERN_DEBUG "Found %02x at %x in OOB for %08x\n",
> - buf[i], i, jeb->offset));
> - ret = 1;
> - goto out;
> +
> +
> + if (!mode) {
> + for (page = 0; page < len; page += sizeof(long)) {
> + unsigned long dat = *(unsigned long *)(&buf[page]);
> + if (dat != -1) {
> + ret = 1;
> + goto out;
> + }
> }
> - }
> + } else {
> + for (current_oob = 0; current_oob < len; current_oob += oob_size) {
> + node = (struct jffs2_unknown_node *)&buf[current_oob + c->fsdata_pos];
> + if (je16_to_cpu(node->magic) == JFFS2_MAGIC_BITMASK) {
> + switch (je16_to_cpu(node->nodetype) & JFFS2_COMPAT_MASK) {
> + case JFFS2_FEATURE_ROCOMPAT:
> + c->flags |= JFFS2_SB_FLAG_RO;
> + if (!(jffs2_is_readonly(c)))
> + return -EROFS;
> +
> + case JFFS2_FEATURE_RWCOMPAT_DELETE:
> + case JFFS2_FEATURE_RWCOMPAT_COPY:
> + for (i = current_oob; i < current_oob + oob_size; i++) {
> + if (i >= current_oob + c->fsdata_pos &&
> + i < current_oob + c->fsdata_pos + c->fsdata_len)
> + continue;
> +
> + if (buf[i] != 0xFF) {
> + ret = 1;
> + goto out;

Sevenfold indentation is not what I call good coding style. Try to
make this stuff simpler. Use "if (!...) continue" and similar
constructs.

> + }
> + }
> + break;
>
> - /* we know, we are aligned :) */
> - for (page = oob_size; page < len; page += sizeof(long)) {
> - unsigned long dat = *(unsigned long *)(&buf[page]);
> - if(dat != -1) {
> - ret = 1;
> - goto out;
> + case JFFS2_FEATURE_INCOMPAT:
> + return -EINVAL;
> + }
> + } else {
> + for (i = current_oob; i < current_oob + oob_size; i++) {
> + if (buf[i] != 0xFF) {
> + ret = 1;
> + goto out;

Same here.

> + }
> + }
> + }
> }
> }

Jörn

--
A surrounded army must be given a way out.
-- Sun Tzu