Hi, Martin Recently I found this patch has other serious issues. So the check-in has to be delayed.
According to UEFI spec, the SimpleFileSystem protocol interface produced must support partial read. But the patch doesn't support it. And I ever said I had made the patch pass UEFI SCT, but after digging into it, I found it's because the UEFI SCT SimpleFileSystem test case has bug as well. Now I am working with SCT owner to solve these bugs together. Thanks Feng -----Original Message----- From: Olivier Martin [mailto:olivier.mar...@arm.com] Sent: Saturday, December 06, 2014 03:19 To: Justen, Jordan L; Tian, Feng Cc: edk2-devel@lists.sourceforge.net Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem module Feng, what is the next step for moving forward with this patch. As I said in a previous email, I am happy with the changes you added to the original patch. Thanks, Olivier > -----Original Message----- > From: Jordan Justen [mailto:jordan.l.jus...@intel.com] > Sent: 13 November 2014 02:50 > To: Tian, Feng; Olivier Martin > Cc: edk2-devel@lists.sourceforge.net > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem > module > > On 2014-11-12 18:10:11, Tian, Feng wrote: > > Jordan, > > > > As Contributed-under and Signed-off-by message are mandatory infos, > > I didn't mention it. > > Yes, it is mandatory that every contribution have this. Therefore, it > should be mentioned with every contribution of code. I usually don't > even look at a contribution that doesn't have these. > > > Ok, let's mend the commit message: > > > > MdeModulePkg/FvSimpleFileSystem: Add a new module to provide access > to executable files in FVs. > > > > This module implements Simple FileSystem protocol over Firmware > Volume (FV). > > EFI Modules included into a FV can be listed and launched from the > > EFI Shell or any other EFI applications. > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Brendan Jackman <brendan.jack...@arm.com> > > Signed-off-by: Feng Tian <feng.t...@intel.com> > > Reviewed-by: Olivier Martin <olivier.mar...@arm.com> > > > > I would prefer to let package maintainer, that's me:), to commit > > code for quality control. Just giving feedback is not enough as we > > have additional works to do. > > I do agree about testing. I would pull in Olivier's change. Test it, > and only then give him a r-b and tell him it is okay for him to commit > the change when his series is ready. > > But, you can handle contributions differently for the packages you > maintain. > > Except, one note. If the contribution had several patches to your > package. In that case you should preserve them as separate patches. > > > For these series patches submitted by Martin, only this one gets > > approval at this time. Others need more discussion. > > Yes. Normally I would think Olivier would get his whole series > reviewed and ready before committing it. Maybe it is okay to split it > in some cases. > > -Jordan > > > -----Original Message----- > > From: Justen, Jordan L > > Sent: Thursday, November 13, 2014 09:25 > > To: Tian, Feng; 'Olivier Martin (olivier.mar...@arm.com)' > > Cc: edk2-devel@lists.sourceforge.net > > Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem > module > > > > On 2014-11-11 23:16:21, Tian, Feng wrote: > > > Hi, Martin > > > > > > Here is my review comments on your FvSimpleFileSystem uefi driver. > Sorry for late response. > > > > > > 1. Fixed system hang due to wrong assertions in > FvSimpleFileSystemDriverStart() and FvSimpleFileSystemOpenVolume(). > > > a. change ASSERT (NumChars == GUID_STRING_SIZE); to ASSERT > ((NumChars + 1) * sizeof (CHAR16) == GUID_STRING_SIZE); > > > b. change ASSERT (NumChars == FVFS_VOLUME_LABEL_SIZE); to > > > ASSERT ((NumChars + 1) * sizeof (CHAR16) == > FVFS_VOLUME_LABEL_SIZE); 2. Fixed SimpleFileSystem SCT errors. > > > a. EFI_FILE_PROTOCOL.GetInfo() should handle VolumeLabel > info rather than return EFI_UNSUPPORTED. > > > b. EFI_FILE_PROTOCOL.GetInfo() should initial Size field > of EFI_FILE_SYSTEM_INFO at run time. > > > c. Some interfaces in EFI_FILE_PROTOCOL should return > EFI_WRITE_PROTECTED rather than EFI_UNSUPPORTED. > > > d. EFI_FILE_PROTOCOL.Delete() should return > EFI_WARN_DELETE_FAILURE rather than EFI_UNSUPPORTED. > > > 3. Added missing ComponentName protocol interface and modify code > to use APIs defined in UefiLib to install DriverBinding and > ComponentName protocol. > > > 4. Fixed wrong UnicodeCollation protocol usage. > > > 5. Added missing function comments. > > > 6. Added missing user extension meta data in INF file. > > > 7. Remove BasePathLib dependency and use internal implementation > as the move operation for this library and others in ShellPkg need > more discussions. > > > > > > I attached the modified path for your review, will check it in to > MdeModulePkg/Universal after pass your review. BasePathLib will be on > other thread. > > > > I wonder what Olivier is supposed to do with this patch. I notice it > lacks a Contributed-under and Signed-off-by for your changes. > > > > I assume you'll add that, and then Olivier will add it to the commit > messages? > > > > Another (I think easier) approach would be to provide feedback and > let Olivier modify his code based on your suggestions. > > > > You might also give your r-b and let Olivier commit the patch series > since he has commit access. (It seems he had a 3 patch series > > originally.) > > > > -Jordan ------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=164703151&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
