Re: [U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset

# 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