| Message ID | 20180904074948.18146-19-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | subject: fs: fat: extend FAT write operations | expand |
On 04.09.18 09:49, AKASHI Takahiro wrote: > This patch is necessary to run SCT.efi (UEFI Self-Certification Test). > Returning EFI_SUCCESS can cheat SCT execution. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> How hard would it be to implement a real delete instead? Alex > --- > lib/efi_loader/efi_file.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c > index 6ec98c80227e..12044a0c7196 100644 > --- a/lib/efi_loader/efi_file.c > +++ b/lib/efi_loader/efi_file.c > @@ -226,12 +226,20 @@ static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file) > return EFI_EXIT(file_close(fh)); > } > > +static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file, > + efi_uintn_t *buffer_size, > + void *buffer); > + > static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file) > { > struct file_handle *fh = to_fh(file); > + efi_uintn_t size = 0; > EFI_ENTRY("%p", file); > + > + /* TODO: implement real 'delete' */ > + efi_file_write(file, &size, NULL); > file_close(fh); > - return EFI_EXIT(EFI_WARN_DELETE_FAILURE); > + return EFI_SUCCESS; > } > > static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size, >
On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote: > > > On 04.09.18 09:49, AKASHI Takahiro wrote: > > This patch is necessary to run SCT.efi (UEFI Self-Certification Test). > > Returning EFI_SUCCESS can cheat SCT execution. > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > How hard would it be to implement a real delete instead? Good question. The hardest part of implementation, I believe, is about handling nasty long file names in a directory, in particular, when directory entries which comprise a single LFN extend across two clusters. In this case, all the entries must be deleted first, and then go back to the first one and modify it. Due to a current simple buffering of cluster (only one cluster buffer a time), some tweak is necessary. In addition, if there is an already-deleted entry ahead of the given file, we need further rewind directory entries and update the "new" last valid one to mark it as so. Those kind of things would make the implementation a bit complicated rather than simple as expected. So unless 'real' delete is a must for anything, I'm lukewarm to work on it. # I admit that we don't have to have this patch just if *delete* returns successfully without doing anything. Thanks, -Takahiro AKASHI > > Alex > > > --- > > lib/efi_loader/efi_file.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c > > index 6ec98c80227e..12044a0c7196 100644 > > --- a/lib/efi_loader/efi_file.c > > +++ b/lib/efi_loader/efi_file.c > > @@ -226,12 +226,20 @@ static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file) > > return EFI_EXIT(file_close(fh)); > > } > > > > +static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file, > > + efi_uintn_t *buffer_size, > > + void *buffer); > > + > > static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file) > > { > > struct file_handle *fh = to_fh(file); > > + efi_uintn_t size = 0; > > EFI_ENTRY("%p", file); > > + > > + /* TODO: implement real 'delete' */ > > + efi_file_write(file, &size, NULL); > > file_close(fh); > > - return EFI_EXIT(EFI_WARN_DELETE_FAILURE); > > + return EFI_SUCCESS; > > } > > > > static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size, > >
On 05.09.18 04:51, AKASHI Takahiro wrote: > On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote: >> >> >> On 04.09.18 09:49, AKASHI Takahiro wrote: >>> This patch is necessary to run SCT.efi (UEFI Self-Certification Test). >>> Returning EFI_SUCCESS can cheat SCT execution. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> >> How hard would it be to implement a real delete instead? > > Good question. > The hardest part of implementation, I believe, is about handling > nasty long file names in a directory, in particular, when > directory entries which comprise a single LFN extend across two clusters. > In this case, all the entries must be deleted first, and then go back > to the first one and modify it. Due to a current simple buffering of > cluster (only one cluster buffer a time), some tweak is necessary. > In addition, if there is an already-deleted entry ahead of the given file, > we need further rewind directory entries and update the "new" last valid > one to mark it as so. > Those kind of things would make the implementation a bit complicated > rather than simple as expected. > > So unless 'real' delete is a must for anything, I'm lukewarm to > work on it. > > # I admit that we don't have to have this patch just if *delete* returns > successfully without doing anything. I think both truncate-to-zero (this patch) and report-success-when-failure (suggested patch) are hacks that shouldn't really go upstream that way unfortunately. The way I read the long file name definitions, I think it should be perfectly ok to only remove the short file name directory entry. Sure, we're leaking a few directory entries for the LFN, but I don't think that's too bad. Any fsck should be able to find those later on... As for how to delete the entry without rewinding directory entries, you can just set name[0]=DELETED_FLAG; to indicate that the SFN is deleted, no? Alex
On Wed, Sep 05, 2018 at 10:22:07AM +0200, Alexander Graf wrote: > > > On 05.09.18 04:51, AKASHI Takahiro wrote: > > On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote: > >> > >> > >> On 04.09.18 09:49, AKASHI Takahiro wrote: > >>> This patch is necessary to run SCT.efi (UEFI Self-Certification Test). > >>> Returning EFI_SUCCESS can cheat SCT execution. > >>> > >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >> > >> How hard would it be to implement a real delete instead? > > > > Good question. > > The hardest part of implementation, I believe, is about handling > > nasty long file names in a directory, in particular, when > > directory entries which comprise a single LFN extend across two clusters. > > In this case, all the entries must be deleted first, and then go back > > to the first one and modify it. Due to a current simple buffering of > > cluster (only one cluster buffer a time), some tweak is necessary. > > In addition, if there is an already-deleted entry ahead of the given file, > > we need further rewind directory entries and update the "new" last valid > > one to mark it as so. > > Those kind of things would make the implementation a bit complicated > > rather than simple as expected. > > > > So unless 'real' delete is a must for anything, I'm lukewarm to > > work on it. > > > > # I admit that we don't have to have this patch just if *delete* returns > > successfully without doing anything. > > I think both truncate-to-zero (this patch) and > report-success-when-failure (suggested patch) are hacks that shouldn't > really go upstream that way unfortunately. > > The way I read the long file name definitions, I think it should be > perfectly ok to only remove the short file name directory entry. Do you accept such an ugly implementation although it yet complies with fat specification? > Sure, > we're leaking a few directory entries for the LFN, but I don't think > that's too bad. Any fsck should be able to find those later on... As I said in my cover-letter, fsck would complain any way. > As for how to delete the entry without rewinding directory entries, you > can just set name[0]=DELETED_FLAG; to indicate that the SFN is deleted, no? Yes if DELETED_FLAG=0x5e and if we ignore the specific case where name[0] be 0x00, which means the entry is the *first* invalid one. (again, the latter would not be mandatory in term of compatibility.) Since I have already had the code, "real" delete will be put in my next version if nobody has concerns on this simple implementation. Thanks, -Takahiro AKASHI > > Alex
> Am 06.09.2018 um 04:43 schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>: > >> On Wed, Sep 05, 2018 at 10:22:07AM +0200, Alexander Graf wrote: >> >> >>> On 05.09.18 04:51, AKASHI Takahiro wrote: >>>> On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote: >>>> >>>> >>>>> On 04.09.18 09:49, AKASHI Takahiro wrote: >>>>> This patch is necessary to run SCT.efi (UEFI Self-Certification Test). >>>>> Returning EFI_SUCCESS can cheat SCT execution. >>>>> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> >>>> How hard would it be to implement a real delete instead? >>> >>> Good question. >>> The hardest part of implementation, I believe, is about handling >>> nasty long file names in a directory, in particular, when >>> directory entries which comprise a single LFN extend across two clusters. >>> In this case, all the entries must be deleted first, and then go back >>> to the first one and modify it. Due to a current simple buffering of >>> cluster (only one cluster buffer a time), some tweak is necessary. >>> In addition, if there is an already-deleted entry ahead of the given file, >>> we need further rewind directory entries and update the "new" last valid >>> one to mark it as so. >>> Those kind of things would make the implementation a bit complicated >>> rather than simple as expected. >>> >>> So unless 'real' delete is a must for anything, I'm lukewarm to >>> work on it. >>> >>> # I admit that we don't have to have this patch just if *delete* returns >>> successfully without doing anything. >> >> I think both truncate-to-zero (this patch) and >> report-success-when-failure (suggested patch) are hacks that shouldn't >> really go upstream that way unfortunately. >> >> The way I read the long file name definitions, I think it should be >> perfectly ok to only remove the short file name directory entry. > > Do you accept such an ugly implementation although it yet complies > with fat specification? It's valid for all intents and purposes, so why not :). > >> Sure, >> we're leaking a few directory entries for the LFN, but I don't think >> that's too bad. Any fsck should be able to find those later on... > > As I said in my cover-letter, fsck would complain any way. > >> As for how to delete the entry without rewinding directory entries, you >> can just set name[0]=DELETED_FLAG; to indicate that the SFN is deleted, no? > > Yes if DELETED_FLAG=0x5e and if we ignore the specific case where > name[0] be 0x00, which means the entry is the *first* invalid one. > (again, the latter would not be mandatory in term of compatibility.) > > Since I have already had the code, "real" delete will be put in my next > version if nobody has concerns on this simple implementation. Thanks, that would be great! :) Alex
diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c index 6ec98c80227e..12044a0c7196 100644 --- a/lib/efi_loader/efi_file.c +++ b/lib/efi_loader/efi_file.c @@ -226,12 +226,20 @@ static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file) return EFI_EXIT(file_close(fh)); } +static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file, + efi_uintn_t *buffer_size, + void *buffer); + static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file) { struct file_handle *fh = to_fh(file); + efi_uintn_t size = 0; EFI_ENTRY("%p", file); + + /* TODO: implement real 'delete' */ + efi_file_write(file, &size, NULL); file_close(fh); - return EFI_EXIT(EFI_WARN_DELETE_FAILURE); + return EFI_SUCCESS; } static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
This patch is necessary to run SCT.efi (UEFI Self-Certification Test). Returning EFI_SUCCESS can cheat SCT execution. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_file.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)