mbox series

[U-Boot,00/17] fs: fat: extend FAT write operations

Message ID 20180720025723.6736-1-takahiro.akashi@linaro.org
Headers show
Series fs: fat: extend FAT write operations | expand

Message

Takahiro Akashi July 20, 2018, 2:57 a.m. UTC
This patch series is an attempt to address FAT write related issues
in an effort of running UEFI SCT (Self-Certification Test) to verify
UEFI support on u-boot.

SCT is a test platform as well as an extentisive collection of test
cases for UEFI specification. It can run all the tests automatically
and save test results to dedicated log files.

AFAIK, what's missing in the current fat file system to safely run
SCT without errors (I don't mean test case failures) are:
* write a file located under sub-directories
* write a file with non-zero offset
* delete a file
* create a directory

Patch#1 to patch#6 are some sort of preparatory ones.
Patch#7 implements write with sub-directories path.
Patch#8 to patch#10 implement write with non-zero offset.
Patch#11 to patch#15 are related to creating a directory.
Patch#16 provides delete, but it doesn't actually delete a file
but instead return 0 with purging a file content, which is good
enough to run SCT for now.
Finally, patch#17 fixes a minor bug in fs-test.sh.

I applied this patch set on top of v2018.07 along with a couple of
yet-to--be-upstreamed UEFI-related patches, and could successfully
run unmodified SCT[1] on qemu-arm.

[1] http://uefi.org/testtools

AKASHI Takahiro (17):
  fs: fat: extend get_fs_info() for write use
  fs: fat: handle "." and ".." of root dir correctly with
    fat_itr_resolve()
  fs: fat: make directory iterator global for write use
  fs: fat: assure iterator's ->dent belongs to ->clust
  fs: fat: check and normailze file name
  fs: fat: write returns error code instead of -1
  fs: fat: support write with sub-directory path
  fs: fat: refactor write interface for a file offset
  fs: fat: support write with non-zero offset
  cmd: fat: add offset parameter to fatwrite
  fs: add mkdir interface
  fs: fat: remember the starting cluster number of directory
  fs: fat: support mkdir
  cmd: fat: add fatmkdir command
  efi_loader: file: support creating a directory
  efi_loader: implement a pseudo "file delete"
  fs-test: fix false positive error at Test Case 12

 cmd/fat.c                 |   22 +-
 fs/fat/fat.c              |   98 ++--
 fs/fat/fat_write.c        | 1051 +++++++++++++++++++++++--------------
 fs/fs.c                   |   46 ++
 include/fat.h             |   37 ++
 include/fs.h              |   10 +
 lib/efi_loader/efi_file.c |   24 +-
 test/fs/fs-test.sh        |    2 +-
 8 files changed, 825 insertions(+), 465 deletions(-)

Comments

Heinrich Schuchardt July 20, 2018, 9:48 a.m. UTC | #1
On 07/20/2018 04:57 AM, AKASHI Takahiro wrote:
> This patch series is an attempt to address FAT write related issues
> in an effort of running UEFI SCT (Self-Certification Test) to verify
> UEFI support on u-boot.
> 
> SCT is a test platform as well as an extentisive collection of test
> cases for UEFI specification. It can run all the tests automatically
> and save test results to dedicated log files.
> 
> AFAIK, what's missing in the current fat file system to safely run
> SCT without errors (I don't mean test case failures) are:
> * write a file located under sub-directories
> * write a file with non-zero offset
> * delete a file
> * create a directory
> 
> Patch#1 to patch#6 are some sort of preparatory ones.
> Patch#7 implements write with sub-directories path.
> Patch#8 to patch#10 implement write with non-zero offset.
> Patch#11 to patch#15 are related to creating a directory.
> Patch#16 provides delete, but it doesn't actually delete a file
> but instead return 0 with purging a file content, which is good
> enough to run SCT for now.
> Finally, patch#17 fixes a minor bug in fs-test.sh.
> 
> I applied this patch set on top of v2018.07 along with a couple of
> yet-to--be-upstreamed UEFI-related patches, and could successfully
> run unmodified SCT[1] on qemu-arm.
> 
> [1] http://uefi.org/testtools

Thanks Takahiro for getting this all implemented.

I have some questions concerning code pages:

The FAT file system contains a short name and a long name. The short 
name is in the 8-bit code page of the system. The long name is 16bit 
Unicode.

Which local code page do you assume? I would suggest to use 437 as we do 
in some other drivers.

With your patch fat_mkdir() and fat_file_write() do not yet support 
UTF-8 as file names and convert this to the 437 local page for the short 
name and to UTF-16 for the long name.

Without this conversoin we will not be able to implement the EFI 
specification.

normalize_longname() simply ignores codes 0x80-0xFF. Please, have a look at

https://www.win.tue.nl/~aeb/linux/fs/fat/fatgen103.pdf
Microsoft Extensible Firmware Initiative FAT32 File System Specification
FAT: General Overview of On-Disk Format
Version 1.03, December 6, 2000

Shouldn't we implement the base-name algorithm as described in the paper?

Best regards

Heinrich
Heinrich Schuchardt July 22, 2018, 6:44 a.m. UTC | #2
Hello Tom, hello Alex,

I have been testing the patches. They are working fine for ASCII file
names. To support Unicode file names extra work will be needed. But
probably we should postpone this to a later patch series.

There are some dependencies with my work for correcting errors in
Unicode handling for the EFI branch. Should the patches be passed via
efi-next?

Best regards

Heinrich
Takahiro Akashi July 23, 2018, 7:35 a.m. UTC | #3
On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> Hello Tom, hello Alex,
> 
> I have been testing the patches. They are working fine for ASCII file
> names. To support Unicode file names extra work will be needed. But
> probably we should postpone this to a later patch series.

I absolutely agree.
To be clear, the aim of my FAT write patchset is to successfully run
SCT to evaluate and verify UEFI-related features rather than to make
the implementation comply with UEFI specification.

For the latter purpose, separate patches would be compiled as it might
require time-consuming efforts.
(I'm sure that SCT's file (media access) protocol test causes lots of
test case failures.)

Thanks,
-Takahiro AKASHI

> There are some dependencies with my work for correcting errors in
> Unicode handling for the EFI branch. Should the patches be passed via
> efi-next?
> 
> Best regards
> 
> Heinrich
Tom Rini July 23, 2018, 2:46 p.m. UTC | #4
On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> Hello Tom, hello Alex,
> 
> I have been testing the patches. They are working fine for ASCII file
> names. To support Unicode file names extra work will be needed. But
> probably we should postpone this to a later patch series.
> 
> There are some dependencies with my work for correcting errors in
> Unicode handling for the EFI branch. Should the patches be passed via
> efi-next?

Yes, a follow-up series makes sense, and yes, efi-next for the patches
themselves sounds fine, thanks!
Heinrich Schuchardt Aug. 6, 2018, 10:34 p.m. UTC | #5
On 07/23/2018 04:46 PM, Tom Rini wrote:
> On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
>> Hello Tom, hello Alex,
>>
>> I have been testing the patches. They are working fine for ASCII file
>> names. To support Unicode file names extra work will be needed. But
>> probably we should postpone this to a later patch series.
>>
>> There are some dependencies with my work for correcting errors in
>> Unicode handling for the EFI branch. Should the patches be passed via
>> efi-next?
> 
> Yes, a follow-up series makes sense, and yes, efi-next for the patches
> themselves sounds fine, thanks!
> 

Hello Takahiro,

could you, please, resubmit the series with the few changes needed so
that Alex can consider them for efi-next.

Cheers

Heinrich
Takahiro Akashi Aug. 7, 2018, 5:38 a.m. UTC | #6
On Tue, Aug 07, 2018 at 12:34:28AM +0200, Heinrich Schuchardt wrote:
> On 07/23/2018 04:46 PM, Tom Rini wrote:
> > On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> >> Hello Tom, hello Alex,
> >>
> >> I have been testing the patches. They are working fine for ASCII file
> >> names. To support Unicode file names extra work will be needed. But
> >> probably we should postpone this to a later patch series.
> >>
> >> There are some dependencies with my work for correcting errors in
> >> Unicode handling for the EFI branch. Should the patches be passed via
> >> efi-next?
> > 
> > Yes, a follow-up series makes sense, and yes, efi-next for the patches
> > themselves sounds fine, thanks!
> > 
> 
> Hello Takahiro,
> 
> could you, please, resubmit the series with the few changes needed so
> that Alex can consider them for efi-next.

Well, I'm sure that my patch set can be applied to v2018.09-rc1 w/o any
changes if your patch (0dc1bfb7302 "fs: fat: cannot write to subdirectories")
is reverted beforehand. As I said, my patch(#5/17) superseded it in terms of
its function.

What's more, I ran fs-test.sh again, and all tests passed, either against
fat16 or fat32. Do we still want to update a test result in fs-test.sh?
(removing it would be the easiest.)

Thanks,
-Takahiro AKASHI


> Cheers
> 
> Heinrich
Takahiro Akashi Aug. 7, 2018, 5:52 a.m. UTC | #7
On Tue, Aug 07, 2018 at 02:38:34PM +0900, AKASHI Takahiro wrote:
> On Tue, Aug 07, 2018 at 12:34:28AM +0200, Heinrich Schuchardt wrote:
> > On 07/23/2018 04:46 PM, Tom Rini wrote:
> > > On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
> > >> Hello Tom, hello Alex,
> > >>
> > >> I have been testing the patches. They are working fine for ASCII file
> > >> names. To support Unicode file names extra work will be needed. But
> > >> probably we should postpone this to a later patch series.
> > >>
> > >> There are some dependencies with my work for correcting errors in
> > >> Unicode handling for the EFI branch. Should the patches be passed via
> > >> efi-next?
> > > 
> > > Yes, a follow-up series makes sense, and yes, efi-next for the patches
> > > themselves sounds fine, thanks!
> > > 
> > 
> > Hello Takahiro,
> > 
> > could you, please, resubmit the series with the few changes needed so
> > that Alex can consider them for efi-next.
> 
> Well, I'm sure that my patch set can be applied to v2018.09-rc1 w/o any
> changes if your patch (0dc1bfb7302 "fs: fat: cannot write to subdirectories")
> is reverted beforehand. As I said, my patch(#5/17) superseded it in terms of
> its function.

See:
https://git.linaro.org/people/takahiro.akashi/u-boot.git fat_write

> What's more, I ran fs-test.sh again, and all tests passed, either against
> fat16 or fat32. Do we still want to update a test result in fs-test.sh?
> (removing it would be the easiest.)

I've almost finished to convert fs-test.sh to a Python script, but
it will take some time to add more tests against my patch set as
my priority right now is on examining a test result from SCT.

-Takahiro AKASHI


> Thanks,
> -Takahiro AKASHI
> 
> 
> > Cheers
> > 
> > Heinrich
Heinrich Schuchardt Aug. 7, 2018, 5:53 a.m. UTC | #8
On 08/07/2018 07:38 AM, AKASHI Takahiro wrote:
> On Tue, Aug 07, 2018 at 12:34:28AM +0200, Heinrich Schuchardt wrote:
>> On 07/23/2018 04:46 PM, Tom Rini wrote:
>>> On Sun, Jul 22, 2018 at 08:44:39AM +0200, Heinrich Schuchardt wrote:
>>>> Hello Tom, hello Alex,
>>>>
>>>> I have been testing the patches. They are working fine for ASCII file
>>>> names. To support Unicode file names extra work will be needed. But
>>>> probably we should postpone this to a later patch series.
>>>>
>>>> There are some dependencies with my work for correcting errors in
>>>> Unicode handling for the EFI branch. Should the patches be passed via
>>>> efi-next?
>>>
>>> Yes, a follow-up series makes sense, and yes, efi-next for the patches
>>> themselves sounds fine, thanks!
>>>
>>
>> Hello Takahiro,
>>
>> could you, please, resubmit the series with the few changes needed so
>> that Alex can consider them for efi-next.
> 
> Well, I'm sure that my patch set can be applied to v2018.09-rc1 w/o any
> changes if your patch (0dc1bfb7302 "fs: fat: cannot write to subdirectories")
> is reverted beforehand. As I said, my patch(#5/17) superseded it in terms of
> its function.

Please, add a patch doing the reverting or rebase patch 5.

> 
> What's more, I ran fs-test.sh again, and all tests passed, either against
> fat16 or fat32. Do we still want to update a test result in fs-test.sh?
> (removing it would be the easiest.)

As long as we have no better tests in place updating those comments is
appropriate.

In my review of patch 01/17 I indicated an error in evaluating the
sector number.

Best regards

Heinrich