Re: [U-Boot] [PATCH] fs: add fs_readdir()
Hi Stefan, On 13 August 2017 at 17:01, Stefan Bruens wrote: > On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote: >> Hi Rob, >> >> On 10 August 2017 at 12:13, Rob Clark wrote: >> > On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass wrote: >> >> Hi Rob, >> >> >> >> On 3 August 2017 at 13:36, Rob Clark wrote: >> >>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >> >>> >> >>> 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 >> > --- >> > >> > 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 0x0108 ./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
Re: [U-Boot] [PATCH] fs: add fs_readdir()
On Sonntag, 13. August 2017 23:36:57 CEST Simon Glass wrote: > Hi Rob, > > On 10 August 2017 at 12:13, Rob Clark wrote: > > On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass wrote: > >> Hi Rob, > >> > >> On 3 August 2017 at 13:36, Rob Clark wrote: > >>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan > >>> > >>> 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 > > --- > > > > 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 0x0108 ./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 -- Stefan Brüns / Bergstraße 21 / 52062 Aachen home: +49 241 53809034 mobile: +49 151 50412019 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fs: add fs_readdir()
Hi Rob, On 10 August 2017 at 12:13, Rob Clark wrote: > On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass wrote: >> Hi Rob, >> >> On 3 August 2017 at 13:36, Rob Clark wrote: >>> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >>> 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 > --- > 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 0x0108 ./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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fs: add fs_readdir()
On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass wrote: > Hi Rob, > > On 3 August 2017 at 13:36, Rob Clark wrote: >> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >> 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 --- 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 0x0108 ./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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fs: add fs_readdir()
On Sun, Aug 6, 2017 at 1:16 AM, Simon Glass wrote: > Hi Rob, > > On 3 August 2017 at 13:36, Rob Clark wrote: >> On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan >> 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 --- 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fs: add fs_readdir()
Hi Rob, On 3 August 2017 at 13:36, Rob Clark wrote: > On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan > 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 >>> --- >>> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fs: add fs_readdir()
On Thu, Aug 3, 2017 at 3:10 PM, Brüns, Stefan 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 >> --- >> 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH] fs: add fs_readdir()
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 > --- > 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 ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
