public inbox for u-boot@lists.denx.de
 help / color / mirror / Atom feed
From: Detlev Zundel <dzu@denx.de>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH V2] JFFS2: bug fix for summary support.
Date: Tue, 19 Apr 2011 17:01:04 +0200	[thread overview]
Message-ID: <m2vcyai8un.fsf@ohwell.denx.de> (raw)
In-Reply-To: <001201cbfca4$2fc3fa80$6401a8c0@LENOVOE5CA6843> (Baidu Liu's message of "Sun, 17 Apr 2011 10:07:02 +0800")

Hi Baidu,

>  This patch fixes some issues with JFFS2 summary support in U-Boot.
>  1/ Bug fix for summary support: we need to get the latest DIRENT.
>  For example, if you create a file in linux jffs2 which config summary
>  support, then you delete the file , you will not see the file  in
>  linux jffs2. But you can also see this file in uboot after you reset
>  the system. That is because all the nodes in jffs2 which config summary
>  will not be marked as obsolete. The deleted file's DIRENT node will be
>  seen in uboot. So what we need to do is to get the latest DIRENT whose
>  ino in DIRENT is 0. Than we will not see this file in uboot which is
>  what we want.
>  2/ Add CONFIG_SYS_JFFS2_SORT_FRAGMENTS definition,if we config jffs2
>  summary in uboot.
>  3/ Avoid allocate too big memory if the biggest file in JFFS2 is too
>  long. We only allocate one node size for pL->readbuf.
>  For example, if the biggest file in the jffs2 is 15MB in the 16MB jffs2
>  system. Our previous code will allocate a buffer(pL->readbuf) as 15MB
>  length.
>  4/ Improve error checking in the scanning.

I missed saying this explicitely in my first review that we should
strive to isolate changes into single commits.  As you have added even
more changes into a single commit this has now become somewhat
untolerable.  Please split into individual functional changes (git add
-i can work wonders here), i.e. the list items in the changelog should
become individual commits.

This also includes the added WATCHDOG_RESET not mentioned in the
changelog at all ;)

> Changes for v2:
>       - Add detail description for the patch.

Are you sure you only changed the description?  The diffstat from this
and the last time disagree - this time:

> ---
>  fs/jffs2/jffs2_1pass.c      |   60 +++++++++++++++++++++++++-----------------
>  fs/jffs2/jffs2_nand_1pass.c |   28 +++++++++++++++-----
>  include/jffs2/jffs2.h       |   10 +++++++
>  3 files changed, 67 insertions(+), 31 deletions(-)

Last time:

>  fs/jffs2/jffs2_1pass.c      |   53 +++++++++++++++++++++++++-----------------
>  fs/jffs2/jffs2_nand_1pass.c |   24 ++++++++++++++-----
>  include/jffs2/jffs2.h       |   11 +++++++++
>  3 files changed, 60 insertions(+), 28 deletions(-)

Looking over the changes, I do _see_ changes in code, so you should tell
us about them.

> [...]

> diff --git a/include/jffs2/jffs2.h b/include/jffs2/jffs2.h
> index 651f94c..5b006c0 100644
> --- a/include/jffs2/jffs2.h
> +++ b/include/jffs2/jffs2.h
> @@ -41,6 +41,16 @@
>  #include <asm/types.h>
>  #include <jffs2/load_kernel.h>
>  
> +#ifdef CONFIG_JFFS2_SUMMARY
> +#ifndef CONFIG_SYS_JFFS2_SORT_FRAGMENTS
> +/*
> +we should define CONFIG_SYS_JFFS2_SORT_FRAGMENTS,if
> +CONFIG_JFFS2_SUMMARY is enabled.
> +*/
> +#define CONFIG_SYS_JFFS2_SORT_FRAGMENTS
> +#endif
> +#endif
> +
>  #define JFFS2_SUPER_MAGIC 0x72b6
>  
>  /* Values we may expect to find in the 'magic' field */

I liked the previous version better:

> diff --git a/include/jffs2/jffs2.h b/include/jffs2/jffs2.h
> index 651f94c..c01a76e 100644
> --- a/include/jffs2/jffs2.h
> +++ b/include/jffs2/jffs2.h
> @@ -41,6 +41,17 @@
>  #include <asm/types.h>
>  #include <jffs2/load_kernel.h>
>  
> +#ifdef CONFIG_JFFS2_SUMMARY
> +#ifndef CONFIG_SYS_JFFS2_SORT_FRAGMENTS
> +/*
> + *	if we define summary in jffs2, we also need to define 
> + *	CONFIG_SYS_JFFS2_SORT_FRAGMENTS. If not, the data in latest inode may be 
> + *  overwritten by the old one.
> +*/
> +#error "need to define CONFIG_SYS_JFFS2_SORT_FRAGMENTS,if summary is enabled"
> +#endif
> +#endif
> +
>  #define JFFS2_SUPER_MAGIC 0x72b6

Why did you change this to the worse?

Cheers
  Detlev

-- 
Q: Why did the chicken cross the Moebius strip?
A: To get to the other ... er, um ...
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

  reply	other threads:[~2011-04-19 15:01 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-04-17  2:07 [U-Boot] [PATCH V2] JFFS2: bug fix for summary support Baidu Liu
2011-04-19 15:01 ` Detlev Zundel [this message]
2011-04-24  3:45   ` Baidu Liu
2011-04-27  9:45     ` Detlev Zundel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=m2vcyai8un.fsf@ohwell.denx.de \
    --to=dzu@denx.de \
    --cc=u-boot@lists.denx.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox