All of lore.kernel.org
 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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.