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, but I will try to prepare some simple
scripts for sandbox to reproduce the observed issues.
>>> Signed-off-by: Marek Szyprowski
>>> ---
>>> 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, )) {
>>> 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 Institute Poland
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
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
> > ---
> > 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, )) {
> > printf("Error get-and-setting cluster\n");
>
> Adding in Heinrich and Akashi-san for more review on this, thanks!
>
> --
> Tom
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset
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
> ---
> 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, )) {
> printf("Error get-and-setting cluster\n");
Adding in Heinrich and Akashi-san for more review on this, thanks!
--
Tom
signature.asc
Description: PGP signature
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
[U-Boot] [PATCH v3 2/6] fat: write: fix broken write at non-zero file offset
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
---
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, )) {
printf("Error get-and-setting cluster\n");
--
2.17.1
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
