| Message ID | 002f01cbf84d$4e0c24a0$6401a8c0@LENOVOE5CA6843 |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Hi Leo, > 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. Can you give an exmaple how this bug showed? > 2/ Avoid allocate too big memory if the biggest file in JFFS2 is too > long. We only allocate one node size for pL->readbuf. > 3/ Free memory space if we fail to scan the JFFS2. > > Signed-off-by: Leo Liu <liucai.lfn@gmail.com> Please fix the obvious style errors: total: 24 errors, 7 warnings, 216 lines checked NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or scripts/cleanfile > --- > 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(-) > > diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c > index c4f7445..dfb1745 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); > } > } > @@ -676,14 +677,19 @@ jffs_init_1pass_list(struct part_info *part) > > if (NULL != (part->jffs2_priv = malloc(sizeof(struct b_lists)))) { > pL = (struct b_lists *)part->jffs2_priv; > - > memset(pL, 0, sizeof(*pL)); > + > + pL->readbuf = malloc(sizeof(union jffs2_node_union)); > + if(!pL->readbuf) { > + printf("jffs_init_1pass_list: malloc failed\n"); > + return 0; > + } > #ifdef CONFIG_SYS_JFFS2_SORT_FRAGMENTS > pL->dir.listCompare = compare_dirents; > pL->frag.listCompare = compare_inodes; > #endif > } > - return 0; > + return 1; Why are you changing return codes here? I cannot see how this matches with the patch description. > } > > /* find the inode from the slashless name given a parent */ > @@ -748,8 +754,8 @@ jffs2_1pass_read_inode(struct b_lists *pL, u32 inode, char *dest) > > if(dest) { > src = ((uchar *) jNode) + sizeof(struct jffs2_raw_inode); > - /* ignore data behind latest known EOF */ > - if (jNode->offset > totalSize) { > + /* ignore data which exceed file length */ > + if (jNode->offset + jNode->dsize > totalSize) { > put_fl_mem(jNode, pL->readbuf); > continue; > } > @@ -835,10 +841,10 @@ jffs2_1pass_find_inode(struct b_lists * pL, const char *name, u32 pino) > for(b = pL->dir.listHead; b; b = b->next, counter++) { > jDir = (struct jffs2_raw_dirent *) get_node_mem(b->offset, > pL->readbuf); > - if ((pino == jDir->pino) && (len == jDir->nsize) && > - (jDir->ino) && /* 0 for unlink */ > + if ((pino == jDir->pino) && > + (len == jDir->nsize) && > (!strncmp((char *)jDir->name, name, len))) { /* a match */ > - if (jDir->version < version) { > + if (jDir->version < version) { /*ignore the old DIRENT*/ > put_fl_mem(jDir, pL->readbuf); > continue; > } > @@ -963,6 +969,13 @@ jffs2_1pass_list_inodes(struct b_lists * pL, u32 pino) > struct jffs2_raw_inode *jNode, *i = NULL; > struct b_node *b2 = pL->frag.listHead; > > + /* > + we compare the DIRENT's ino with the latest DIRENT's ino t determine whether this DIRENT > + is the latest. If the DIRENT is not the latest,ignore it. > + */ > + if(jDir->ino != jffs2_1pass_find_inode(pL, jDir->name, pino)) > + continue; > + > while (b2) { > jNode = (struct jffs2_raw_inode *) > get_fl_mem(b2->offset, sizeof(ojNode), &ojNode); > @@ -1448,7 +1461,6 @@ jffs2_1pass_build_lists(struct part_info * part) > u32 counter4 = 0; > u32 counterF = 0; > u32 counterN = 0; > - u32 max_totlen = 0; > u32 buf_size = DEFAULT_EMPTY_SCAN_SIZE; > char *buf; > > @@ -1458,9 +1470,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); > - pL = (struct b_lists *)part->jffs2_priv; > + 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 */ Ah I see, jffs_init_1pass_list can now make the process fail. This is worth mentioning in the commit log, i.e. "improve error checking" or something. After all, this changes how the code behaves, so it should be documented. Cheers Detlev
Hi, Detlev: 2011/4/13 Detlev Zundel <dzu@denx.de>: > Hi Leo, > >> 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. > > Can you give an exmaple how this bug showed? > 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 support will not be marked as obsolete. The delete 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. So we will not see this file in uboot which is what we want. >> 2/ Avoid allocate too big memory if the biggest file in JFFS2 is too >> long. We only allocate one node size for pL->readbuf. >> 3/ Free memory space if we fail to scan the JFFS2. >> >> Signed-off-by: Leo Liu <liucai.lfn@gmail.com> > > Please fix the obvious style errors: > > total: 24 errors, 7 warnings, 216 lines checked > > NOTE: whitespace errors detected, you may wish to use scripts/cleanpatch or > scripts/cleanfile > I will send a new patch, thanks >> --- >> 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(-) >> >> diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c >> index c4f7445..dfb1745 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); >> } >> } >> @@ -676,14 +677,19 @@ jffs_init_1pass_list(struct part_info *part) >> >> if (NULL != (part->jffs2_priv = malloc(sizeof(struct b_lists)))) { >> pL = (struct b_lists *)part->jffs2_priv; >> - >> memset(pL, 0, sizeof(*pL)); >> + >> + pL->readbuf = malloc(sizeof(union jffs2_node_union)); >> + if(!pL->readbuf) { >> + printf("jffs_init_1pass_list: malloc failed\n"); >> + return 0; >> + } >> #ifdef CONFIG_SYS_JFFS2_SORT_FRAGMENTS >> pL->dir.listCompare = compare_dirents; >> pL->frag.listCompare = compare_inodes; >> #endif >> } >> - return 0; >> + return 1; > > Why are you changing return codes here? I cannot see how this matches > with the patch description. we may failed when we do malloc, so we need to check the return value of the malloc. >> } >> >> /* find the inode from the slashless name given a parent */ >> @@ -748,8 +754,8 @@ jffs2_1pass_read_inode(struct b_lists *pL, u32 inode, char *dest) >> >> if(dest) { >> src = ((uchar *) jNode) + sizeof(struct jffs2_raw_inode); >> - /* ignore data behind latest known EOF */ >> - if (jNode->offset > totalSize) { >> + /* ignore data which exceed file length */ >> + if (jNode->offset + jNode->dsize > totalSize) { >> put_fl_mem(jNode, pL->readbuf); >> continue; >> } >> @@ -835,10 +841,10 @@ jffs2_1pass_find_inode(struct b_lists * pL, const char *name, u32 pino) >> for(b = pL->dir.listHead; b; b = b->next, counter++) { >> jDir = (struct jffs2_raw_dirent *) get_node_mem(b->offset, >> pL->readbuf); >> - if ((pino == jDir->pino) && (len == jDir->nsize) && >> - (jDir->ino) && /* 0 for unlink */ actually , this is the key point of this patch >> + if ((pino == jDir->pino) && >> + (len == jDir->nsize) && >> (!strncmp((char *)jDir->name, name, len))) { /* a match */ >> - if (jDir->version < version) { >> + if (jDir->version < version) { /*ignore the old DIRENT*/ >> put_fl_mem(jDir, pL->readbuf); >> continue; >> } >> @@ -963,6 +969,13 @@ jffs2_1pass_list_inodes(struct b_lists * pL, u32 pino) >> struct jffs2_raw_inode *jNode, *i = NULL; >> struct b_node *b2 = pL->frag.listHead; >> >> + /* >> + we compare the DIRENT's ino with the latest DIRENT's ino t determine whether this DIRENT >> + is the latest. If the DIRENT is not the latest,ignore it. >> + */ >> + if(jDir->ino != jffs2_1pass_find_inode(pL, jDir->name, pino)) >> + continue; actually , this is the key point of this patch >> + >> while (b2) { >> jNode = (struct jffs2_raw_inode *) >> get_fl_mem(b2->offset, sizeof(ojNode), &ojNode); >> @@ -1448,7 +1461,6 @@ jffs2_1pass_build_lists(struct part_info * part) >> u32 counter4 = 0; >> u32 counterF = 0; >> u32 counterN = 0; >> - u32 max_totlen = 0; >> u32 buf_size = DEFAULT_EMPTY_SCAN_SIZE; >> char *buf; >> >> @@ -1458,9 +1470,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); >> - pL = (struct b_lists *)part->jffs2_priv; >> + 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 */ > > Ah I see, jffs_init_1pass_list can now make the process fail. This is > worth mentioning in the commit log, i.e. "improve error checking" or > something. After all, this changes how the code behaves, so it should > be documented. > I will do it in the next patch, thanks > Cheers > Detlev > > -- > NAN - No Acronym Neccessary > -- > 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@denx.de >
Hi Baidu, > Hi, Detlev: > > 2011/4/13 Detlev Zundel <dzu@denx.de>: >> Hi Leo, >> >>> 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. >> >> Can you give an exmaple how this bug showed? >> > 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 support > will not be marked as obsolete. The delete 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. So we will not see this file in uboot which is > what we want. Ok, I see. Please include this in the commit message in the next round. Cheers Detlev
diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c index c4f7445..dfb1745 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); } } @@ -676,14 +677,19 @@ jffs_init_1pass_list(struct part_info *part) if (NULL != (part->jffs2_priv = malloc(sizeof(struct b_lists)))) { pL = (struct b_lists *)part->jffs2_priv; - memset(pL, 0, sizeof(*pL)); + + pL->readbuf = malloc(sizeof(union jffs2_node_union)); + if(!pL->readbuf) { + printf("jffs_init_1pass_list: malloc failed\n"); + return 0; + } #ifdef CONFIG_SYS_JFFS2_SORT_FRAGMENTS pL->dir.listCompare = compare_dirents; pL->frag.listCompare = compare_inodes; #endif } - return 0; + return 1; } /* find the inode from the slashless name given a parent */ @@ -748,8 +754,8 @@ jffs2_1pass_read_inode(struct b_lists *pL, u32 inode, char *dest) if(dest) { src = ((uchar *) jNode) + sizeof(struct jffs2_raw_inode); - /* ignore data behind latest known EOF */ - if (jNode->offset > totalSize) { + /* ignore data which exceed file length */ + if (jNode->offset + jNode->dsize > totalSize) { put_fl_mem(jNode, pL->readbuf); continue; } @@ -835,10 +841,10 @@ jffs2_1pass_find_inode(struct b_lists * pL, const char *name, u32 pino) for(b = pL->dir.listHead; b; b = b->next, counter++) { jDir = (struct jffs2_raw_dirent *) get_node_mem(b->offset, pL->readbuf); - if ((pino == jDir->pino) && (len == jDir->nsize) && - (jDir->ino) && /* 0 for unlink */ + if ((pino == jDir->pino) && + (len == jDir->nsize) && (!strncmp((char *)jDir->name, name, len))) { /* a match */ - if (jDir->version < version) { + if (jDir->version < version) { /*ignore the old DIRENT*/ put_fl_mem(jDir, pL->readbuf); continue; } @@ -963,6 +969,13 @@ jffs2_1pass_list_inodes(struct b_lists * pL, u32 pino) struct jffs2_raw_inode *jNode, *i = NULL; struct b_node *b2 = pL->frag.listHead; + /* + we compare the DIRENT's ino with the latest DIRENT's ino t determine whether this DIRENT + is the latest. If the DIRENT is not the latest,ignore it. + */ + if(jDir->ino != jffs2_1pass_find_inode(pL, jDir->name, pino)) + continue; + while (b2) { jNode = (struct jffs2_raw_inode *) get_fl_mem(b2->offset, sizeof(ojNode), &ojNode); @@ -1448,7 +1461,6 @@ jffs2_1pass_build_lists(struct part_info * part) u32 counter4 = 0; u32 counterF = 0; u32 counterN = 0; - u32 max_totlen = 0; u32 buf_size = DEFAULT_EMPTY_SCAN_SIZE; char *buf; @@ -1458,9 +1470,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); - pL = (struct b_lists *)part->jffs2_priv; + 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 */ @@ -1520,7 +1539,7 @@ jffs2_1pass_build_lists(struct part_info * part) ret = jffs2_sum_scan_sumnode(part, sector_ofs, sumptr, sumlen, pL); - if (buf_size && sumlen > buf_size) + if (sumlen > buf_size) free(sumptr); if (ret < 0) { free(buf); @@ -1645,8 +1664,6 @@ jffs2_1pass_build_lists(struct part_info * part) jffs2_free_cache(part); return 0; } - if (max_totlen < node->totlen) - max_totlen = node->totlen; break; case JFFS2_NODETYPE_DIRENT: if (buf_ofs + buf_len < ofs + sizeof(struct @@ -1675,8 +1692,6 @@ jffs2_1pass_build_lists(struct part_info * part) jffs2_free_cache(part); return 0; } - if (max_totlen < node->totlen) - max_totlen = node->totlen; counterN++; break; case JFFS2_NODETYPE_CLEANMARKER: @@ -1708,12 +1723,6 @@ jffs2_1pass_build_lists(struct part_info * part) free(buf); putstr("\b\b done.\r\n"); /* close off the dots */ - /* We don't care if malloc failed - then each read operation will - * allocate its own buffer as necessary (NAND) or will read directly - * from flash (NOR). - */ - pL->readbuf = malloc(max_totlen); - /* turn the lcd back on. */ /* splash(); */ diff --git a/fs/jffs2/jffs2_nand_1pass.c b/fs/jffs2/jffs2_nand_1pass.c index 3982003..e3bc536 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; } @@ -305,8 +306,8 @@ jffs2_1pass_read_inode(struct b_lists *pL, u32 ino, char *dest, if (dest) len += jNode->csize; nand_read(nand, jNode->offset, &len, inode); - /* ignore data behind latest known EOF */ - if (inode->offset > totalSize) + /* ignore data which exceed file length */ + if (inode->offset + inode->dsize> totalSize) continue; if (stat) { @@ -371,7 +372,7 @@ jffs2_1pass_find_inode(struct b_lists * pL, const char *name, u32 pino) /* we need to search all and return the inode with the highest version */ for (jDir = (struct b_dirent *)pL->dir.listHead; jDir; jDir = jDir->next) { - if ((pino == jDir->pino) && (jDir->ino) && /* 0 for unlink */ + if ((pino == jDir->pino) && (len == jDir->nsize) && (nhash == jDir->nhash)) { /* TODO: compare name */ if (jDir->version < version) @@ -483,6 +484,14 @@ jffs2_1pass_list_inodes(struct b_lists * pL, u32 pino) struct b_inode *jNode = (struct b_inode *)pL->frag.listHead; struct b_inode *i = NULL; + /* + if the DIRENT is not the latest,ignore it. + we compare the DIRENT's ino with the latest DIRENT's ino to determine whether this DIRENT + is the latest. + */ + if(jDir.ino != jffs2_1pass_find_inode(pL, jDir->name, pino)) + continue; + while (jNode) { if (jNode->ino == jDir->ino && jNode->version >= i_version) { i_version = jNode->version; @@ -797,7 +806,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: "); @@ -809,6 +820,8 @@ jffs2_1pass_build_lists(struct part_info * part) return 0; for (i = 0; i < nr_blocks; i++) { + WATCHDOG_RESET(); + printf("\b\b%c ", spinner[counter++ % sizeof(spinner)]); offset = part->offset + i * sectorsize; @@ -878,6 +891,7 @@ jffs2_1pass_build_lists(struct part_info * part) } } + free(buf); putstr("\b\b done.\r\n"); /* close off the dots */ #if 0 @@ -897,8 +911,6 @@ jffs2_1pass_build_lists(struct part_info * part) /* give visual feedback that we are done scanning the flash */ led_blink(0x0, 0x0, 0x1, 0x1); /* off, forever, on 100ms, off 100ms */ - free(buf); - return 1; } 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 /* Values we may expect to find in the 'magic' field */
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. 2/ Avoid allocate too big memory if the biggest file in JFFS2 is too long. We only allocate one node size for pL->readbuf. 3/ Free memory space if we fail to scan the JFFS2. Signed-off-by: Leo Liu <liucai.lfn@gmail.com> --- 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(-)