| Message ID | 20170803165438.11494-1-robdclark@gmail.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: > Needed to support efi file protocol. The fallback.efi loader wants > to be able to read the contents of the /EFI directory to find an OS > to boot. > > Currently only implemented for FAT, but that is all that UEFI is > required to support. > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > fs/fat/fat.c | 59 > ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | > 25 +++++++++++++++++++++++++ > include/fat.h | 4 +++- > include/fs.h | 21 +++++++++++++++++++++ > 4 files changed, 95 insertions(+), 14 deletions(-) > NAK 1. The current code is already much to convoluted. Your changes add to this significantly 2. Your patch description neither references the exact part of the EFI specification you want to support (which took me some time, for reference it is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you specifying the required semantics (which is "open", "read", "close", where each read returns a single directory entry, similar to the POSIX opendir(), readdir() calls. 3. Usage of an index too lookup the next entry is also quite convoluted. 4. As far as I can see, your code will fail to find files in the root directory (look for LS_ROOT). I think it would be much better to first restructure the current code to use an readdir like interface internally, and then do everything EFI needs on top. This would get rid of the 4 almost identical copies to print the current directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining directory traversal and and also avoid the bug in (4.). Kind regards, Stefan
On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan <Stefan.Bruens@rwth-aachen.de> wrote: > On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >> Needed to support efi file protocol. The fallback.efi loader wants >> to be able to read the contents of the /EFI directory to find an OS >> to boot. >> >> Currently only implemented for FAT, but that is all that UEFI is >> required to support. >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> >> --- >> fs/fat/fat.c | 59 >> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | >> 25 +++++++++++++++++++++++++ >> include/fat.h | 4 +++- >> include/fs.h | 21 +++++++++++++++++++++ >> 4 files changed, 95 insertions(+), 14 deletions(-) >> > > NAK > > 1. The current code is already much to convoluted. Your changes add to this > significantly I agree with the first part of that statement, but not the second. The code is pretty awful, but apparently works for people, and I don't know (or have the time to learn) enough about FAT to do a massive re-write. I'll split this patch so we can add the interface without FAT implementation, and I'll just carry around the second part downstream. > 2. Your patch description neither references the exact part of the EFI > specification you want to support (which took me some time, for reference it > is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you > specifying the required semantics (which is "open", "read", "close", where > each read returns a single directory entry, similar to the POSIX opendir(), > readdir() calls. I can add a note in the commit message.. although I didn't really think it would be too relevant to this patch. (More relevant to the patch which adds the efi_loader part, which depends on this patch.) > 3. Usage of an index too lookup the next entry is also quite convoluted. > > 4. As far as I can see, your code will fail to find files in the root > directory (look for LS_ROOT). You could be right.. nothing ever traverses the root directory. > I think it would be much better to first restructure the current code to use > an readdir like interface internally, and then do everything EFI needs on top. tbh, it would be nice even to implement fs_ls() generically on top of readdir().. although I didn't do that since it would be slower (without a re-write of FAT implementation, since we currently have to re-traverse things for each readdir()). BR, -R > This would get rid of the 4 almost identical copies to print the current > directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining > directory traversal and and also avoid the bug in (4.). > > Kind regards, > > Stefan
Hi Rob, On 3 August 2017 at 13:36, Rob Clark <robdclark@gmail.com> wrote: > On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan > <Stefan.Bruens@rwth-aachen.de> wrote: >> On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >>> Needed to support efi file protocol. The fallback.efi loader wants >>> to be able to read the contents of the /EFI directory to find an OS >>> to boot. >>> >>> Currently only implemented for FAT, but that is all that UEFI is >>> required to support. >>> >>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>> --- >>> fs/fat/fat.c | 59 >>> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | >>> 25 +++++++++++++++++++++++++ >>> include/fat.h | 4 +++- >>> include/fs.h | 21 +++++++++++++++++++++ >>> 4 files changed, 95 insertions(+), 14 deletions(-) >>> >> >> NAK >> >> 1. The current code is already much to convoluted. Your changes add to this >> significantly > > I agree with the first part of that statement, but not the second. > The code is pretty awful, but apparently works for people, and I don't > know (or have the time to learn) enough about FAT to do a massive > re-write. > > I'll split this patch so we can add the interface without FAT > implementation, and I'll just carry around the second part downstream. > >> 2. Your patch description neither references the exact part of the EFI >> specification you want to support (which took me some time, for reference it >> is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you >> specifying the required semantics (which is "open", "read", "close", where >> each read returns a single directory entry, similar to the POSIX opendir(), >> readdir() calls. > > I can add a note in the commit message.. although I didn't really > think it would be too relevant to this patch. (More relevant to the > patch which adds the efi_loader part, which depends on this patch.) > >> 3. Usage of an index too lookup the next entry is also quite convoluted. >> >> 4. As far as I can see, your code will fail to find files in the root >> directory (look for LS_ROOT). > > You could be right.. nothing ever traverses the root directory. > >> I think it would be much better to first restructure the current code to use >> an readdir like interface internally, and then do everything EFI needs on top. > > tbh, it would be nice even to implement fs_ls() generically on top of > readdir().. although I didn't do that since it would be slower > (without a re-write of FAT implementation, since we currently have to > re-traverse things for each readdir()). > > BR, > -R > >> This would get rid of the 4 almost identical copies to print the current >> directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining >> directory traversal and and also avoid the bug in (4.). >> >> Kind regards, >> >> Stefan How can we get some tests for this code? We have fs-tests.sh - perhaps we should build on that? If it helps I could bring that into the pytest framework and you could take it from there? With tests we at least have the possibility of refactoring later. Regards, Simon
On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Rob, > > On 3 August 2017 at 13:36, Rob Clark <robdclark@gmail.com> wrote: >> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >> <Stefan.Bruens@rwth-aachen.de> wrote: >>> On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >>>> Needed to support efi file protocol. The fallback.efi loader wants >>>> to be able to read the contents of the /EFI directory to find an OS >>>> to boot. >>>> >>>> Currently only implemented for FAT, but that is all that UEFI is >>>> required to support. >>>> >>>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>>> --- >>>> fs/fat/fat.c | 59 >>>> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | >>>> 25 +++++++++++++++++++++++++ >>>> include/fat.h | 4 +++- >>>> include/fs.h | 21 +++++++++++++++++++++ >>>> 4 files changed, 95 insertions(+), 14 deletions(-) >>>> >>> >>> NAK >>> >>> 1. The current code is already much to convoluted. Your changes add to this >>> significantly >> >> I agree with the first part of that statement, but not the second. >> The code is pretty awful, but apparently works for people, and I don't >> know (or have the time to learn) enough about FAT to do a massive >> re-write. >> >> I'll split this patch so we can add the interface without FAT >> implementation, and I'll just carry around the second part downstream. >> >>> 2. Your patch description neither references the exact part of the EFI >>> specification you want to support (which took me some time, for reference it >>> is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you >>> specifying the required semantics (which is "open", "read", "close", where >>> each read returns a single directory entry, similar to the POSIX opendir(), >>> readdir() calls. >> >> I can add a note in the commit message.. although I didn't really >> think it would be too relevant to this patch. (More relevant to the >> patch which adds the efi_loader part, which depends on this patch.) >> >>> 3. Usage of an index too lookup the next entry is also quite convoluted. >>> >>> 4. As far as I can see, your code will fail to find files in the root >>> directory (look for LS_ROOT). >> >> You could be right.. nothing ever traverses the root directory. >> >>> I think it would be much better to first restructure the current code to use >>> an readdir like interface internally, and then do everything EFI needs on top. >> >> tbh, it would be nice even to implement fs_ls() generically on top of >> readdir().. although I didn't do that since it would be slower >> (without a re-write of FAT implementation, since we currently have to >> re-traverse things for each readdir()). >> >> BR, >> -R >> >>> This would get rid of the 4 almost identical copies to print the current >>> directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining >>> directory traversal and and also avoid the bug in (4.). >>> >>> Kind regards, >>> >>> Stefan > > How can we get some tests for this code? We have fs-tests.sh - perhaps > we should build on that? If it helps I could bring that into the > pytest framework and you could take it from there? > > With tests we at least have the possibility of refactoring later. > Hmm, good question. I'm not super-familiar with how the test framework works.. I guess it would need something exposed in u-boot scripting environment? I suppose I could implement a ls2 command that does the same thing as ls but using fs_readdir()? BR, -R
On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote: > Hi Rob, > > On 3 August 2017 at 13:36, Rob Clark <robdclark@gmail.com> wrote: >> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >> <Stefan.Bruens@rwth-aachen.de> wrote: >>> On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >>>> Needed to support efi file protocol. The fallback.efi loader wants >>>> to be able to read the contents of the /EFI directory to find an OS >>>> to boot. >>>> >>>> Currently only implemented for FAT, but that is all that UEFI is >>>> required to support. >>>> >>>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>>> --- >>>> fs/fat/fat.c | 59 >>>> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | >>>> 25 +++++++++++++++++++++++++ >>>> include/fat.h | 4 +++- >>>> include/fs.h | 21 +++++++++++++++++++++ >>>> 4 files changed, 95 insertions(+), 14 deletions(-) >>>> >>> >>> NAK >>> >>> 1. The current code is already much to convoluted. Your changes add to this >>> significantly >> >> I agree with the first part of that statement, but not the second. >> The code is pretty awful, but apparently works for people, and I don't >> know (or have the time to learn) enough about FAT to do a massive >> re-write. >> >> I'll split this patch so we can add the interface without FAT >> implementation, and I'll just carry around the second part downstream. >> >>> 2. Your patch description neither references the exact part of the EFI >>> specification you want to support (which took me some time, for reference it >>> is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you >>> specifying the required semantics (which is "open", "read", "close", where >>> each read returns a single directory entry, similar to the POSIX opendir(), >>> readdir() calls. >> >> I can add a note in the commit message.. although I didn't really >> think it would be too relevant to this patch. (More relevant to the >> patch which adds the efi_loader part, which depends on this patch.) >> >>> 3. Usage of an index too lookup the next entry is also quite convoluted. >>> >>> 4. As far as I can see, your code will fail to find files in the root >>> directory (look for LS_ROOT). >> >> You could be right.. nothing ever traverses the root directory. >> >>> I think it would be much better to first restructure the current code to use >>> an readdir like interface internally, and then do everything EFI needs on top. >> >> tbh, it would be nice even to implement fs_ls() generically on top of >> readdir().. although I didn't do that since it would be slower >> (without a re-write of FAT implementation, since we currently have to >> re-traverse things for each readdir()). >> >> BR, >> -R >> >>> This would get rid of the 4 almost identical copies to print the current >>> directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining >>> directory traversal and and also avoid the bug in (4.). >>> >>> Kind regards, >>> >>> Stefan > > How can we get some tests for this code? We have fs-tests.sh - perhaps > we should build on that? If it helps I could bring that into the > pytest framework and you could take it from there? > > With tests we at least have the possibility of refactoring later. > So I haven't had a whole lot of luck getting fs-tests.sh working properly (on master).. With the ext4 tests, at some point mounting the loopback image fails, I end up with this in dmesg: EXT4-fs (loop0): ext4_check_descriptors: Checksum for group 0 failed (50995!=31053) EXT4-fs (loop0): group descriptors corrupted! I guess technically I don't need to run ext4 tests, so if I skip those and just run the fat tests, I still end up with some fails with things like: => fatload host 0:0 0x01000008 ./1MB.file.w2 ** Unable to read file ./1MB.file.w2 ** I'm not sure if this is down to some differences in my environment, or if these tests just don't get run often? What I have done is add a ls2 cmd, which implements ls on top of fs_readdir(), which would at least make testing possible. (And fixed a few bugs that turns up with some manual testing.) BR, -R
Hi Rob, On 10 August 2017 at 12:13, Rob Clark <robdclark@gmail.com> wrote: > On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote: >> Hi Rob, >> >> On 3 August 2017 at 13:36, Rob Clark <robdclark@gmail.com> wrote: >>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >>> <Stefan.Bruens@rwth-aachen.de> wrote: >>>> On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >>>>> Needed to support efi file protocol. The fallback.efi loader wants >>>>> to be able to read the contents of the /EFI directory to find an OS >>>>> to boot. >>>>> >>>>> Currently only implemented for FAT, but that is all that UEFI is >>>>> required to support. >>>>> >>>>> Signed-off-by: Rob Clark <robdclark@gmail.com> >>>>> --- >>>>> fs/fat/fat.c | 59 >>>>> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | >>>>> 25 +++++++++++++++++++++++++ >>>>> include/fat.h | 4 +++- >>>>> include/fs.h | 21 +++++++++++++++++++++ >>>>> 4 files changed, 95 insertions(+), 14 deletions(-) >>>>> >>>> >>>> NAK >>>> >>>> 1. The current code is already much to convoluted. Your changes add to this >>>> significantly >>> >>> I agree with the first part of that statement, but not the second. >>> The code is pretty awful, but apparently works for people, and I don't >>> know (or have the time to learn) enough about FAT to do a massive >>> re-write. >>> >>> I'll split this patch so we can add the interface without FAT >>> implementation, and I'll just carry around the second part downstream. >>> >>>> 2. Your patch description neither references the exact part of the EFI >>>> specification you want to support (which took me some time, for reference it >>>> is "13.: Protocols - Media Access, 13.5: File Protocol"), nor are you >>>> specifying the required semantics (which is "open", "read", "close", where >>>> each read returns a single directory entry, similar to the POSIX opendir(), >>>> readdir() calls. >>> >>> I can add a note in the commit message.. although I didn't really >>> think it would be too relevant to this patch. (More relevant to the >>> patch which adds the efi_loader part, which depends on this patch.) >>> >>>> 3. Usage of an index too lookup the next entry is also quite convoluted. >>>> >>>> 4. As far as I can see, your code will fail to find files in the root >>>> directory (look for LS_ROOT). >>> >>> You could be right.. nothing ever traverses the root directory. >>> >>>> I think it would be much better to first restructure the current code to use >>>> an readdir like interface internally, and then do everything EFI needs on top. >>> >>> tbh, it would be nice even to implement fs_ls() generically on top of >>> readdir().. although I didn't do that since it would be slower >>> (without a re-write of FAT implementation, since we currently have to >>> re-traverse things for each readdir()). >>> >>> BR, >>> -R >>> >>>> This would get rid of the 4 almost identical copies to print the current >>>> directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the remaining >>>> directory traversal and and also avoid the bug in (4.). >>>> >>>> Kind regards, >>>> >>>> Stefan >> >> How can we get some tests for this code? We have fs-tests.sh - perhaps >> we should build on that? If it helps I could bring that into the >> pytest framework and you could take it from there? >> >> With tests we at least have the possibility of refactoring later. >> > > So I haven't had a whole lot of luck getting fs-tests.sh working > properly (on master).. > > With the ext4 tests, at some point mounting the loopback image fails, > I end up with this in dmesg: > > EXT4-fs (loop0): ext4_check_descriptors: Checksum for group 0 failed > (50995!=31053) > EXT4-fs (loop0): group descriptors corrupted! I haven't seen that one before! > > I guess technically I don't need to run ext4 tests, so if I skip those > and just run the fat tests, I still end up with some fails with things > like: > > => fatload host 0:0 0x01000008 ./1MB.file.w2 > ** Unable to read file ./1MB.file.w2 ** > > I'm not sure if this is down to some differences in my environment, or > if these tests just don't get run often? It could be either We should convert this to the pytest framework so that it will be run on each pull request.. > > What I have done is add a ls2 cmd, which implements ls on top of > fs_readdir(), which would at least make testing possible. (And fixed > a few bugs that turns up with some manual testing.) OK, well I don't have any great ideas. Do you have time to convert the test? I just sent patches to convert test/image/test-fit.py > > BR, > -R Regards, Simon
On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote: > Hi Rob, > > On 10 August 2017 at 12:13, Rob Clark <robdclark@gmail.com> wrote: > > On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote: > >> Hi Rob, > >> > >> On 3 August 2017 at 13:36, Rob Clark <robdclark@gmail.com> wrote: > >>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan > >>> > >>> <Stefan.Bruens@rwth-aachen.de> wrote: > >>>> On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: > >>>>> Needed to support efi file protocol. The fallback.efi loader wants > >>>>> to be able to read the contents of the /EFI directory to find an OS > >>>>> to boot. > >>>>> > >>>>> Currently only implemented for FAT, but that is all that UEFI is > >>>>> required to support. > >>>>> > >>>>> Signed-off-by: Rob Clark <robdclark@gmail.com> > >>>>> --- > >>>>> > >>>>> fs/fat/fat.c | 59 > >>>>> > >>>>> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c > >>>>> | > >>>>> 25 +++++++++++++++++++++++++ > >>>>> > >>>>> include/fat.h | 4 +++- > >>>>> include/fs.h | 21 +++++++++++++++++++++ > >>>>> 4 files changed, 95 insertions(+), 14 deletions(-) > >>>> > >>>> NAK > >>>> > >>>> 1. The current code is already much to convoluted. Your changes add to > >>>> this > >>>> significantly > >>> > >>> I agree with the first part of that statement, but not the second. > >>> The code is pretty awful, but apparently works for people, and I don't > >>> know (or have the time to learn) enough about FAT to do a massive > >>> re-write. > >>> > >>> I'll split this patch so we can add the interface without FAT > >>> implementation, and I'll just carry around the second part downstream. > >>> > >>>> 2. Your patch description neither references the exact part of the EFI > >>>> specification you want to support (which took me some time, for > >>>> reference it is "13.: Protocols - Media Access, 13.5: File Protocol"), > >>>> nor are you specifying the required semantics (which is "open", > >>>> "read", "close", where each read returns a single directory entry, > >>>> similar to the POSIX opendir(), readdir() calls. > >>> > >>> I can add a note in the commit message.. although I didn't really > >>> think it would be too relevant to this patch. (More relevant to the > >>> patch which adds the efi_loader part, which depends on this patch.) > >>> > >>>> 3. Usage of an index too lookup the next entry is also quite > >>>> convoluted. > >>>> > >>>> 4. As far as I can see, your code will fail to find files in the root > >>>> directory (look for LS_ROOT). > >>> > >>> You could be right.. nothing ever traverses the root directory. > >>> > >>>> I think it would be much better to first restructure the current code > >>>> to use an readdir like interface internally, and then do everything > >>>> EFI needs on top.>>> > >>> tbh, it would be nice even to implement fs_ls() generically on top of > >>> readdir().. although I didn't do that since it would be slower > >>> (without a re-write of FAT implementation, since we currently have to > >>> re-traverse things for each readdir()). > >>> > >>> BR, > >>> -R > >>> > >>>> This would get rid of the 4 almost identical copies to print the > >>>> current > >>>> directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the > >>>> remaining directory traversal and and also avoid the bug in (4.). > >>>> > >>>> Kind regards, > >>>> > >>>> Stefan > >> > >> How can we get some tests for this code? We have fs-tests.sh - perhaps > >> we should build on that? If it helps I could bring that into the > >> pytest framework and you could take it from there? > >> > >> With tests we at least have the possibility of refactoring later. > > > > So I haven't had a whole lot of luck getting fs-tests.sh working > > properly (on master).. > > > > With the ext4 tests, at some point mounting the loopback image fails, > > > > I end up with this in dmesg: > > EXT4-fs (loop0): ext4_check_descriptors: Checksum for group 0 failed > > > > (50995!=31053) > > > > EXT4-fs (loop0): group descriptors corrupted! > > I haven't seen that one before! > > > I guess technically I don't need to run ext4 tests, so if I skip those > > and just run the fat tests, I still end up with some fails with things > > > > like: > > => fatload host 0:0 0x01000008 ./1MB.file.w2 > > ** Unable to read file ./1MB.file.w2 ** > > > > I'm not sure if this is down to some differences in my environment, or > > if these tests just don't get run often? > > It could be either We should convert this to the pytest framework so > that it will be run on each pull request.. You might have forgotten, but I sent a quite large initial implementation of pytest fstests a year ago. These where largely rejected, as these still depends on the ability to run as run to create the images. Regards, Stefan
Hi Stefan, On 13 August 2017 at 17:01, Stefan Bruens <stefan.bruens@rwth-aachen.de> wrote: > On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote: >> Hi Rob, >> >> On 10 August 2017 at 12:13, Rob Clark <robdclark@gmail.com> wrote: >> > On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass <sjg@chromium.org> wrote: >> >> Hi Rob, >> >> >> >> On 3 August 2017 at 13:36, Rob Clark <robdclark@gmail.com> wrote: >> >>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >> >>> >> >>> <Stefan.Bruens@rwth-aachen.de> wrote: >> >>>> On Donnerstag, 3. August 2017 18:54:30 CEST Rob Clark wrote: >> >>>>> Needed to support efi file protocol. The fallback.efi loader wants >> >>>>> to be able to read the contents of the /EFI directory to find an OS >> >>>>> to boot. >> >>>>> >> >>>>> Currently only implemented for FAT, but that is all that UEFI is >> >>>>> required to support. >> >>>>> >> >>>>> Signed-off-by: Rob Clark <robdclark@gmail.com> >> >>>>> --- >> >>>>> >> >>>>> fs/fat/fat.c | 59 >> >>>>> >> >>>>> ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c >> >>>>> | >> >>>>> 25 +++++++++++++++++++++++++ >> >>>>> >> >>>>> include/fat.h | 4 +++- >> >>>>> include/fs.h | 21 +++++++++++++++++++++ >> >>>>> 4 files changed, 95 insertions(+), 14 deletions(-) >> >>>> >> >>>> NAK >> >>>> >> >>>> 1. The current code is already much to convoluted. Your changes add to >> >>>> this >> >>>> significantly >> >>> >> >>> I agree with the first part of that statement, but not the second. >> >>> The code is pretty awful, but apparently works for people, and I don't >> >>> know (or have the time to learn) enough about FAT to do a massive >> >>> re-write. >> >>> >> >>> I'll split this patch so we can add the interface without FAT >> >>> implementation, and I'll just carry around the second part downstream. >> >>> >> >>>> 2. Your patch description neither references the exact part of the EFI >> >>>> specification you want to support (which took me some time, for >> >>>> reference it is "13.: Protocols - Media Access, 13.5: File Protocol"), >> >>>> nor are you specifying the required semantics (which is "open", >> >>>> "read", "close", where each read returns a single directory entry, >> >>>> similar to the POSIX opendir(), readdir() calls. >> >>> >> >>> I can add a note in the commit message.. although I didn't really >> >>> think it would be too relevant to this patch. (More relevant to the >> >>> patch which adds the efi_loader part, which depends on this patch.) >> >>> >> >>>> 3. Usage of an index too lookup the next entry is also quite >> >>>> convoluted. >> >>>> >> >>>> 4. As far as I can see, your code will fail to find files in the root >> >>>> directory (look for LS_ROOT). >> >>> >> >>> You could be right.. nothing ever traverses the root directory. >> >>> >> >>>> I think it would be much better to first restructure the current code >> >>>> to use an readdir like interface internally, and then do everything >> >>>> EFI needs on top.>>> >> >>> tbh, it would be nice even to implement fs_ls() generically on top of >> >>> readdir().. although I didn't do that since it would be slower >> >>> (without a re-write of FAT implementation, since we currently have to >> >>> re-traverse things for each readdir()). >> >>> >> >>> BR, >> >>> -R >> >>> >> >>>> This would get rid of the 4 almost identical copies to print the >> >>>> current >> >>>> directory entry (dols == LS_ROOT || dols == LS_YES), 2 copies of the >> >>>> remaining directory traversal and and also avoid the bug in (4.). >> >>>> >> >>>> Kind regards, >> >>>> >> >>>> Stefan >> >> >> >> How can we get some tests for this code? We have fs-tests.sh - perhaps >> >> we should build on that? If it helps I could bring that into the >> >> pytest framework and you could take it from there? >> >> >> >> With tests we at least have the possibility of refactoring later. >> > >> > So I haven't had a whole lot of luck getting fs-tests.sh working >> > properly (on master).. >> > >> > With the ext4 tests, at some point mounting the loopback image fails, >> > >> > I end up with this in dmesg: >> > EXT4-fs (loop0): ext4_check_descriptors: Checksum for group 0 failed >> > >> > (50995!=31053) >> > >> > EXT4-fs (loop0): group descriptors corrupted! >> >> I haven't seen that one before! >> >> > I guess technically I don't need to run ext4 tests, so if I skip those >> > and just run the fat tests, I still end up with some fails with things >> > >> > like: >> > => fatload host 0:0 0x01000008 ./1MB.file.w2 >> > ** Unable to read file ./1MB.file.w2 ** >> > >> > I'm not sure if this is down to some differences in my environment, or >> > if these tests just don't get run often? >> >> It could be either We should convert this to the pytest framework so >> that it will be run on each pull request.. > > You might have forgotten, but I sent a quite large initial implementation of > pytest fstests a year ago. These where largely rejected, as these still > depends on the ability to run as run to create the images. Run as root? I don't see a problem with that (e.g. to use sudo). Some tests require this. Regards, Simon
diff --git a/fs/fat/fat.c b/fs/fat/fat.c index 9ad18f96ff..04d8616598 100644 --- a/fs/fat/fat.c +++ b/fs/fat/fat.c @@ -14,6 +14,7 @@ #include <config.h> #include <exports.h> #include <fat.h> +#include <fs.h> #include <asm/byteorder.h> #include <part.h> #include <malloc.h> @@ -575,17 +576,25 @@ static __u8 mkcksum(const char name[8], const char ext[3]) /* * Get the directory entry associated with 'filename' from the directory * starting at 'startsect' + * + * Last two args are only used for dols==LS_READDIR */ __u8 get_dentfromdir_block[MAX_CLUSTSIZE] __aligned(ARCH_DMA_MINALIGN); -static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, - char *filename, dir_entry *retdent, - int dols) +static dir_entry *get_dentfromdir(fsdata *mydata, char *filename, + dir_entry *retdent, int dols, + loff_t pos, struct fs_dirent *d) { __u16 prevcksum = 0xffff; __u32 curclust = START(retdent); int files = 0, dirs = 0; + int readdir = 0; + + if (dols == LS_READDIR) { + readdir = 1; + dols = 0; + } debug("get_dentfromdir: %s\n", filename); @@ -618,7 +627,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, get_vfatname(mydata, curclust, get_dentfromdir_block, dentptr, l_name); - if (dols) { + if (dols || readdir) { int isdir; char dirc; int doit = 0; @@ -637,7 +646,14 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, } } if (doit) { - if (dirc == ' ') { + if (readdir) { + if ((dirs + files - 1) == pos) { + strcpy(d->name, l_name); + if (!isdir) + d->size = FAT2CPU32(dentptr->size); + return NULL; + } + } else if (dirc == ' ') { printf(" %8u %s%c\n", FAT2CPU32(dentptr->size), l_name, @@ -676,7 +692,7 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, } get_name(dentptr, s_name); - if (dols) { + if (dols || readdir) { int isdir = (dentptr->attr & ATTR_DIR); char dirc; int doit = 0; @@ -694,7 +710,14 @@ static dir_entry *get_dentfromdir(fsdata *mydata, int startsect, } if (doit) { - if (dirc == ' ') { + if (readdir) { + if ((dirs + files - 1) == pos) { + strcpy(d->name, s_name); + if (!isdir) + d->size = FAT2CPU32(dentptr->size); + return NULL; + } + } else if (dirc == ' ') { printf(" %8u %s%c\n", FAT2CPU32(dentptr->size), s_name, dirc); @@ -825,7 +848,7 @@ int do_fat_read_at(const char *filename, loff_t pos, void *buffer, __u32 cursect; int idx, isdir = 0; int files = 0, dirs = 0; - int ret = -1; + int ret = (dols == LS_READDIR) ? -ENOTDIR : -1; int firsttime; __u32 root_cluster = 0; __u32 read_blk; @@ -906,7 +929,8 @@ root_reparse: if (!dols) goto exit; - dols = LS_ROOT; + if (dols == LS_YES) + dols = LS_ROOT; } else if ((idx = dirdelim(fnamecopy)) >= 0) { isdir = 1; fnamecopy[idx] = '\0'; @@ -1151,8 +1175,6 @@ rootdir_done: firsttime = 1; while (isdir) { - int startsect = mydata->data_begin - + START(dentptr) * mydata->clust_size; dir_entry dent; char *nextname = NULL; @@ -1177,10 +1199,14 @@ rootdir_done: } } - if (get_dentfromdir(mydata, startsect, subname, dentptr, - isdir ? 0 : dols) == NULL) { + if (get_dentfromdir(mydata, subname, dentptr, + isdir ? 0 : dols, pos, buffer) == NULL) { if (dols && !isdir) *size = 0; + if (dols == LS_READDIR) { + struct fs_dirent *dent = buffer; + ret = dent->name[0] ? 0 : -ENOENT; + } goto exit; } @@ -1353,6 +1379,13 @@ int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len, return ret; } +int fat_readdir(const char *filename, loff_t offset, struct fs_dirent *dent) +{ + loff_t actread; + return do_fat_read_at(filename, offset, dent, sizeof(*dent), + LS_READDIR, 0, &actread); +} + void fat_close(void) { } diff --git a/fs/fs.c b/fs/fs.c index 595ff1fe69..42a028a6ce 100644 --- a/fs/fs.c +++ b/fs/fs.c @@ -69,6 +69,12 @@ static inline int fs_uuid_unsupported(char *uuid_str) return -1; } +static inline int fs_readdir_unsupported(const char *filename, loff_t offset, + struct fs_dirent *dent) +{ + return -ENXIO; +} + struct fstype_info { int fstype; char *name; @@ -92,6 +98,7 @@ struct fstype_info { loff_t len, loff_t *actwrite); void (*close)(void); int (*uuid)(char *uuid_str); + int (*readdir)(const char *filename, loff_t offset, struct fs_dirent *dent); }; static struct fstype_info fstypes[] = { @@ -112,6 +119,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, #endif .uuid = fs_uuid_unsupported, + .readdir = fat_readdir, }, #endif #ifdef CONFIG_FS_EXT4 @@ -131,6 +139,7 @@ static struct fstype_info fstypes[] = { .write = fs_write_unsupported, #endif .uuid = ext4fs_uuid, + .readdir = fs_readdir_unsupported, }, #endif #ifdef CONFIG_SANDBOX @@ -146,6 +155,7 @@ static struct fstype_info fstypes[] = { .read = fs_read_sandbox, .write = fs_write_sandbox, .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, #endif #ifdef CONFIG_CMD_UBIFS @@ -161,6 +171,7 @@ static struct fstype_info fstypes[] = { .read = ubifs_read, .write = fs_write_unsupported, .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, #endif { @@ -175,6 +186,7 @@ static struct fstype_info fstypes[] = { .read = fs_read_unsupported, .write = fs_write_unsupported, .uuid = fs_uuid_unsupported, + .readdir = fs_readdir_unsupported, }, }; @@ -334,6 +346,19 @@ int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len, return ret; } +int fs_readdir(const char *filename, loff_t offset, struct fs_dirent *dent) +{ + struct fstype_info *info = fs_get_info(fs_type); + int ret; + + memset(dent, 0, sizeof(*dent)); + + ret = info->readdir(filename, offset, dent); + fs_close(); + + return ret; +} + int do_size(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[], int fstype) { diff --git a/include/fat.h b/include/fat.h index 71879f01ca..0ef3f5be16 100644 --- a/include/fat.h +++ b/include/fat.h @@ -61,8 +61,8 @@ /* Flags telling whether we should read a file or list a directory */ #define LS_NO 0 #define LS_YES 1 -#define LS_DIR 1 #define LS_ROOT 2 +#define LS_READDIR 3 /* read directory entry at specified offset */ #define ISDIRDELIM(c) ((c) == '/' || (c) == '\\') @@ -210,5 +210,7 @@ int file_fat_write(const char *filename, void *buf, loff_t offset, loff_t len, loff_t *actwrite); int fat_read_file(const char *filename, void *buf, loff_t offset, loff_t len, loff_t *actread); +struct fs_dirent; +int fat_readdir(const char *filename, loff_t offset, struct fs_dirent *dir); void fat_close(void); #endif /* _FAT_H_ */ diff --git a/include/fs.h b/include/fs.h index 2f2aca8378..71051d7c66 100644 --- a/include/fs.h +++ b/include/fs.h @@ -79,6 +79,27 @@ int fs_write(const char *filename, ulong addr, loff_t offset, loff_t len, loff_t *actwrite); /* + * A directory entry. + */ +struct fs_dirent { + loff_t size; + char name[256]; +}; + +/* + * fs_readdir - Read a directory. + * + * @filename: Name of file to read from + * @offset: The offset into the directory to read, ie. offset of N returns + * the N'th directory entry + * @dent: on success, filled in with directory entry + * @return 0 on success, -ENOTDIR if specified file is not a directory, + * or -ENOENT if offset is beyond last directory entry, or -ENXIO if + * operation is not supported. + */ +int fs_readdir(const char *filename, loff_t offset, struct fs_dirent *dent); + +/* * Common implementation for various filesystem commands, optionally limited * to a specific filesystem type via the fstype parameter. */
Needed to support efi file protocol. The fallback.efi loader wants to be able to read the contents of the /EFI directory to find an OS to boot. Currently only implemented for FAT, but that is all that UEFI is required to support. Signed-off-by: Rob Clark <robdclark@gmail.com> --- fs/fat/fat.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++------------- fs/fs.c | 25 +++++++++++++++++++++++++ include/fat.h | 4 +++- include/fs.h | 21 +++++++++++++++++++++ 4 files changed, 95 insertions(+), 14 deletions(-)