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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

# 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