* [U-Boot] [PATCH 4/7] JFFS2: Improve error checking @ 2011-04-24 3:37 Baidu Liu 2011-04-29 13:18 ` Detlev Zundel 0 siblings, 1 reply; 4+ messages in thread From: Baidu Liu @ 2011-04-24 3:37 UTC (permalink / raw) To: u-boot Check the return value when we do malloc. Signed-off-by: Baidu Liu <liucai.lfn@gmail.com> --- fs/jffs2/jffs2_1pass.c | 12 ++++++++++-- fs/jffs2/jffs2_nand_1pass.c | 5 ++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c index be6ac78..b3d94af 100644 --- a/fs/jffs2/jffs2_1pass.c +++ b/fs/jffs2/jffs2_1pass.c @@ -662,7 +662,8 @@ jffs2_free_cache(struct part_info *part) pL = (struct b_lists *)part->jffs2_priv; free_nodes(&pL->frag); free_nodes(&pL->dir); - free(pL->readbuf); + if(pL->readbuf) + free(pL->readbuf); free(pL); } } @@ -1470,9 +1471,16 @@ jffs2_1pass_build_lists(struct part_info * part) /* lcd_off(); */ /* if we are building a list we need to refresh the cache. */ - jffs_init_1pass_list(part); + if(! jffs_init_1pass_list(part)) + return 0; + pL = (struct b_lists *)part->jffs2_priv; buf = malloc(buf_size); + if (!buf) { + printf("jffs2_1pass_build_lists: malloc failed\n"); + return 0; + } + puts ("Scanning JFFS2 FS: "); /* start at the beginning of the partition */ diff --git a/fs/jffs2/jffs2_nand_1pass.c b/fs/jffs2/jffs2_nand_1pass.c index 9bad690..885fa3c 100644 --- a/fs/jffs2/jffs2_nand_1pass.c +++ b/fs/jffs2/jffs2_nand_1pass.c @@ -251,6 +251,7 @@ jffs_init_1pass_list(struct part_info *part) pL->dir.listCompare = compare_dirents; pL->frag.listCompare = compare_inodes; #endif + return 1; } return 0; } @@ -806,7 +807,9 @@ jffs2_1pass_build_lists(struct part_info * part) nand = nand_info + id->num; /* if we are building a list we need to refresh the cache. */ - jffs_init_1pass_list(part); + if(! jffs_init_1pass_list(part)) + return 0; + pL = (struct b_lists *)part->jffs2_priv; pL->partOffset = part->offset; puts ("Scanning JFFS2 FS: "); -- 1.7.3.1.msysgit.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 4/7] JFFS2: Improve error checking 2011-04-24 3:37 [U-Boot] [PATCH 4/7] JFFS2: Improve error checking Baidu Liu @ 2011-04-29 13:18 ` Detlev Zundel 2011-04-29 15:02 ` Baidu Liu 0 siblings, 1 reply; 4+ messages in thread From: Detlev Zundel @ 2011-04-29 13:18 UTC (permalink / raw) To: u-boot Hi Baidu, > Check the return value when we do malloc. > > Signed-off-by: Baidu Liu <liucai.lfn@gmail.com> > --- > fs/jffs2/jffs2_1pass.c | 12 ++++++++++-- > fs/jffs2/jffs2_nand_1pass.c | 5 ++++- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c > index be6ac78..b3d94af 100644 > --- a/fs/jffs2/jffs2_1pass.c > +++ b/fs/jffs2/jffs2_1pass.c > @@ -662,7 +662,8 @@ jffs2_free_cache(struct part_info *part) > pL = (struct b_lists *)part->jffs2_priv; > free_nodes(&pL->frag); > free_nodes(&pL->dir); > - free(pL->readbuf); > + if(pL->readbuf) > + free(pL->readbuf); > free(pL); > } > } This looks ok. > @@ -1470,9 +1471,16 @@ jffs2_1pass_build_lists(struct part_info * part) > /* lcd_off(); */ > > /* if we are building a list we need to refresh the cache. */ > - jffs_init_1pass_list(part); > + if(! jffs_init_1pass_list(part)) > + return 0; > + This is strange. We now check for an error of jffs2_init_1pass_list, which currently always returns 0, so let's see where you change that. Ah, you don't (it's in line 671 in this file). It's only in jffs2_nand_1pass that you do a change, but even there we have a problem: > diff --git a/fs/jffs2/jffs2_nand_1pass.c b/fs/jffs2/jffs2_nand_1pass.c > index 9bad690..885fa3c 100644 > --- a/fs/jffs2/jffs2_nand_1pass.c > +++ b/fs/jffs2/jffs2_nand_1pass.c > @@ -251,6 +251,7 @@ jffs_init_1pass_list(struct part_info *part) > pL->dir.listCompare = compare_dirents; > pL->frag.listCompare = compare_inodes; > #endif > + return 1; When malloc fails, we get no error output. > } > return 0; > } > @@ -806,7 +807,9 @@ jffs2_1pass_build_lists(struct part_info * part) > nand = nand_info + id->num; > > /* if we are building a list we need to refresh the cache. */ > - jffs_init_1pass_list(part); > + if(! jffs_init_1pass_list(part)) > + return 0; > + And the caller fails also, so the user in this case will see no error message and no output. Not good. Cheers Detlev -- F?r jemanden, der in eine Religion geboren wurde, in der das Ringen um eine einzige Seele ein Stafettenlauf ?ber viele Jahrhunderte sein kann [..], hat das Tempo des Christentums etwas Schwindelerregendes. Wenn der Hinduismus friedlich dahinflie?t wie der Ganges, dann ist das Christentum Toronto in der Rushhour. -- Yann Martel -- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 4/7] JFFS2: Improve error checking 2011-04-29 13:18 ` Detlev Zundel @ 2011-04-29 15:02 ` Baidu Liu 2011-04-29 17:31 ` Detlev Zundel 0 siblings, 1 reply; 4+ messages in thread From: Baidu Liu @ 2011-04-29 15:02 UTC (permalink / raw) To: u-boot Hi, Detlev >> @@ -1470,9 +1471,16 @@ jffs2_1pass_build_lists(struct part_info * part) >> ? ? ? /* lcd_off(); */ >> >> ? ? ? /* if we are building a list we need to refresh the cache. */ >> - ? ? jffs_init_1pass_list(part); >> + ? ? if(! jffs_init_1pass_list(part)) >> + ? ? ? ? ? ? return 0; >> + > > This is strange. ?We now check for an error of jffs2_init_1pass_list, > which currently always returns 0, so let's see where you change that. > Ah, you don't (it's in line 671 in this file). ?It's only in > jffs2_nand_1pass that you do a change, but even there we have a problem: Yes, we check the return value of function jffs_init_1pass_list(). Also we add the check in nand flash. I do not konw what you are talking about. >> diff --git a/fs/jffs2/jffs2_nand_1pass.c b/fs/jffs2/jffs2_nand_1pass.c >> index 9bad690..885fa3c 100644 >> --- a/fs/jffs2/jffs2_nand_1pass.c >> +++ b/fs/jffs2/jffs2_nand_1pass.c >> @@ -251,6 +251,7 @@ jffs_init_1pass_list(struct part_info *part) >> ? ? ? ? ? ? ? pL->dir.listCompare = compare_dirents; >> ? ? ? ? ? ? ? pL->frag.listCompare = compare_inodes; >> ?#endif >> + ? ? ? ? ? ? return 1; > > When malloc fails, we get no error output. You are too strict. Search the malloc in uboot. There are many places which do not even check the return value. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 4/7] JFFS2: Improve error checking 2011-04-29 15:02 ` Baidu Liu @ 2011-04-29 17:31 ` Detlev Zundel 0 siblings, 0 replies; 4+ messages in thread From: Detlev Zundel @ 2011-04-29 17:31 UTC (permalink / raw) To: u-boot Hi Baidu, > Hi, Detlev > > >>> @@ -1470,9 +1471,16 @@ jffs2_1pass_build_lists(struct part_info * part) >>> ? ? ? /* lcd_off(); */ >>> >>> ? ? ? /* if we are building a list we need to refresh the cache. */ >>> - ? ? jffs_init_1pass_list(part); >>> + ? ? if(! jffs_init_1pass_list(part)) >>> + ? ? ? ? ? ? return 0; >>> + >> >> This is strange. ?We now check for an error of jffs2_init_1pass_list, >> which currently always returns 0, so let's see where you change that. >> Ah, you don't (it's in line 671 in this file). ?It's only in >> jffs2_nand_1pass that you do a change, but even there we have a problem: > Yes, we check the return value of function jffs_init_1pass_list(). > Also we add the check in nand flash. > I do not konw what you are talking about. The function 'jffs_init_1pass_list' is implemented in two files, i.e. in fs/jffs2/jffs2_1pass.c and in fs/jffs2/jffs2_nand_1pass.c. Your patch inserts the actual malloc error checking only in the latter file, whereas the check for return code is done in both files. Just look at your changes - how could your new test in jffs2_1pass ever fail as you did not change the called function? This is _plain inconsistent_ - you missed to do the same error checking for the NOR flash case. >>> diff --git a/fs/jffs2/jffs2_nand_1pass.c b/fs/jffs2/jffs2_nand_1pass.c >>> index 9bad690..885fa3c 100644 >>> --- a/fs/jffs2/jffs2_nand_1pass.c >>> +++ b/fs/jffs2/jffs2_nand_1pass.c >>> @@ -251,6 +251,7 @@ jffs_init_1pass_list(struct part_info *part) >>> ? ? ? ? ? ? ? pL->dir.listCompare = compare_dirents; >>> ? ? ? ? ? ? ? pL->frag.listCompare = compare_inodes; >>> ?#endif >>> + ? ? ? ? ? ? return 1; >> >> When malloc fails, we get no error output. > You are too strict. Search the malloc in uboot. There are many places which > do not even check the return value. This is getting more and more ridiculous. When writing new code, we always have to conform to our own standards. Now you insert an error check but if it fails, you do not inform the user that he ran out of memory but simply provide empty output? So there is _no way_ of telling a failed malloc from an empty directory? And to argument for this broken behavious you point to other places that do no error checking? Sorry, you are loosing my interest of reviewing your code. Best wishes Detlev -- Peace of mind isn't at all superficial to technical work. It's the whole thing. That which produces it is good work and that which destroys it is bad work. -- Robert M. Pirsig -- 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-04-29 17:31 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-24 3:37 [U-Boot] [PATCH 4/7] JFFS2: Improve error checking Baidu Liu 2011-04-29 13:18 ` Detlev Zundel 2011-04-29 15:02 ` Baidu Liu 2011-04-29 17:31 ` Detlev Zundel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox