# I still need to understand the issues reported here. On Tue, Nov 26, 2019 at 11:57:34AM -0500, Tom Rini wrote: > On Tue, Nov 26, 2019 at 09:15:08AM +0100, Marek Szyprowski wrote: > > > Handling of the start file offset was broken in the current code. Although > > the code skipped the needed clusters, it then tried to continue write with > > current cluster set to EOF, what caused assertion. It also lacked adjusting > > filesize in case of writing at the end of file and adjusting in-cluster > > offset for partial overwrite. > > > > This patch fixes all those issues.
If those issues are logically independent from each other, it would be nice to split this patch into small ones. I would like to expect you to add more test cases, especially against corner cases that you mentioned above, to test/py/tests/est_fs as I did in test_ext.py. Or at least please add more assertion checks. > > Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com> > > --- > > fs/fat/fat_write.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c > > index 6cfa5b4565..7fb373589d 100644 > > --- a/fs/fat/fat_write.c > > +++ b/fs/fat/fat_write.c > > @@ -756,14 +756,12 @@ set_contents(fsdata *mydata, dir_entry *dentptr, > > loff_t pos, __u8 *buffer, > > /* go to cluster at pos */ > > cur_pos = bytesperclust; > > while (1) { > > + newclust = get_fatent(mydata, curclust); > > if (pos <= cur_pos) I think that we should change this condition as if (pos < cur_pos) break; then modify the following code accordingly as well. In this way, 'curclust' points to [cur_pos - bytesperclust, cur_pos) and 'pos' is ensured to be in the middle after this 'while' unless (pos == cur_pos) && IS_LAST_CLUST(curclust,...). Then the code will be expected to look better understandable. Thanks, -Takahiro Akashi > > break; > > - if (IS_LAST_CLUST(curclust, mydata->fatsize)) > > + if (IS_LAST_CLUST(newclust, mydata->fatsize)) > > break; > > - > > - newclust = get_fatent(mydata, curclust); > > - if (!IS_LAST_CLUST(newclust, mydata->fatsize) && > > - CHECK_CLUST(newclust, mydata->fatsize)) { > > + if (CHECK_CLUST(newclust, mydata->fatsize)) { > > debug("curclust: 0x%x\n", curclust); > > debug("Invalid FAT entry\n"); > > return -1; > > @@ -772,8 +770,8 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t > > pos, __u8 *buffer, > > cur_pos += bytesperclust; > > curclust = newclust; > > } > > - if (IS_LAST_CLUST(curclust, mydata->fatsize)) { > > - assert(pos == cur_pos); > > + if (pos == cur_pos && IS_LAST_CLUST(newclust, mydata->fatsize)) { > > + filesize -= pos; > > goto set_clusters; > > } > > > > @@ -814,6 +812,7 @@ set_contents(fsdata *mydata, dir_entry *dentptr, loff_t > > pos, __u8 *buffer, > > else > > offset = pos - cur_pos; > > wsize = min_t(unsigned long long, actsize, filesize - cur_pos); > > + wsize -= offset; > > if (get_set_cluster(mydata, curclust, offset, > > buffer, wsize, &actsize)) { > > printf("Error get-and-setting cluster\n"); > > Adding in Heinrich and Akashi-san for more review on this, thanks! > > -- > Tom _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot