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

[Marek Szyprowski]

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 at 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
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland