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

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