| Message ID | 20191126081512.5138-3-m.szyprowski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Matthias Brugger |
| Headers | show |
| Series | Raspberry Pi4: add support for DFU over USB | expand |
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. > > Signed-off-by: Marek Szyprowski <m.szyprowski@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) > 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!
# 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.szyprowski@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
Hi On 27.11.2019 04:13, AKASHI Takahiro wrote: > # 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. Okay, I will try to prepare some tests which show bugs fixed by this patch. I'm not sure I will manage to split this patch into patches fixing each single issue I've observed, because at least some of them were related. I'm not familiar with py_test&co, but I will try to prepare some simple scripts for sandbox to reproduce the observed issues. >>> Signed-off-by: Marek Szyprowski <m.szyprowski@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! Best regards
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) 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");
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. Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- fs/fat/fat_write.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)