Hi, Martin Per EDKII Coding Style 2.0 Spec, STATIC is not recommended for EDKII use. (if you look into MdeModulePkg and MdePkg, you will find nobody uses this keyword as well).
For concern about why I create FV_FILESYSTEM_FILE for every Open() operation, yes, it's mainly because of the case you mentioned. It's mandatory although UEFI spec doesn't say anything about this. Otherwise we have no way to get correct Position info for every Open() if we only return the same EFI_FILE_PROTOCOL instance. Other comments are good to me. I will fix those typos and comment errors at check-in. Last, for review concern, Jodran's suggestion to send changes based on original patch is a good idea. I will follow up it later. For check-in process concern, it's mainly because we have strict check on new contributed modules and interface changes proposed adding to EDKII core packages. We couldn't check in a new module which doesn't follow UEFI spec to core package until we fix it. The 1st revision of the patch for UEFI SCT pass and the 2nd revision of the patch for partial read are the cases. Thanks Feng -----Original Message----- From: Olivier Martin [mailto:olivier.mar...@arm.com] Sent: Thursday, December 18, 2014 20:37 To: Tian, Feng; Justen, Jordan L; Jin, Eric Cc: edk2-devel@lists.sourceforge.net; Colin Drake Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem module Missing 'STATIC' to FvFsFindExecutableSection(), FvFsGetFileInfo(), RemoveLastItemFromPath(), TrimFilePathToAbsolutePath(), InitializeUnicodeCollationSupportWorker() declarations. In FvSimpleFileSystemOpen(), you allocate a new FV_FILESYSTEM_FILE structure. It means if a file is opened twice in the same time there would be two instances of FV_FILESYSTEM_FILE. I do not disagree with the approach - for instance it allows to open the file in read only in by one driver and in read/write by an another driver. The UEFI specification does not say anything about concurrent accesses to a same EFI_FILE_PROTOCOL instance. The only comment I have is in FvSimpleFileSystemInternal.h, FV_FILESYSTEM_FILE is commented such as "Struct representing a file. There will be one of these for each opening file on each FV". We could definitely have more than "one of these". There is typo issue line 909 of FvSimpleFilesystem.c "Return Volume Lable" -> "Return Volume Label" Renamed variable 'FsVolumeLab' into 'FsVolumeLabel' Line 919 (same comment line 899), you wrote: ASSERT_EFI_ERROR (Status); return EFI_SUCCESS; You should write: ASSERT_EFI_ERROR (Status); return Status; As Jordan said, it would have be easier to push my first patch after reviewing it and then to add your new features as new patches. > -----Original Message----- > From: Tian, Feng [mailto:feng.t...@intel.com] > Sent: 18 December 2014 08:47 > To: Justen, Jordan L; Olivier Martin; Jin, Eric > Cc: edk2-devel@lists.sourceforge.net; Colin Drake; Tian, Feng > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem > module > > Jordan > > As I know, there have had several different implementations for > FvSimpleFileSystem besides you mentioned. But till now, only ARM > officially proposed to add this feature to MdeModulePkg. So I have to > use their version rather than others. > > I agree zip file is not a good way for review, here is the .patch file. > > As for commit log, it has been the beginning of the mail thread. No > change on it. > > Thanks > Feng > > -----Original Message----- > From: Justen, Jordan L > Sent: Thursday, December 18, 2014 3:50 AM > To: Tian, Feng; Olivier Martin; Jin, Eric > Cc: edk2-devel@lists.sourceforge.net; Colin Drake > Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem > module > > On 2014-12-16 23:29:05, Tian, Feng wrote: > > Martin, > > > > Please review the latest patch again, > > Once again, you did not supply a Contributed-under or Signed-off-by. > > > in which partial read is supported. > > I think Colin's version already supported this: > https://github.com/cfdrake/FileSystemPkg > > struct _FILE_PRIVATE_DATA.Position: > https://github.com/cfdrake/FileSystemPkg/blob/master/FfsDxe/Ffs.h#L118 > > FfsRead: > https://github.com/cfdrake/FileSystemPkg/blob/master/FfsDxe/Ffs.c#L930 > > > Password: efi;0000 > > I don't think a password zip file is a good way to provide patches. > > I think the best response would have been to reply to Olivier's > original patch, and point out changes that should be made. This way > ARM could make the changes, and we wouldn't have a munged up copyright > situation. > > A second best approach would be to apply Olivier's patch, and then > make a new patch of your own based on his patch. (Then we could easily > see what you changed from his.) > > At the least, it seems like you need to add Contributed-under and > Signed-off-by to be appended to Olivier's patch commit message. > > -Jordan > > > -----Original Message----- > > From: Tian, Feng > > Sent: Monday, December 08, 2014 09:28 > > To: Olivier Martin; Justen, Jordan L; Jin, Eric > > Cc: edk2-devel@lists.sourceforge.net; Tian, Feng > > Subject: RE: [edk2] patch review feedback about FvSimpleFileSystem > > module > > > > 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 > > CAUTION: Please confirm that the password protected .zip attachment > > which contains the file(s) of type > > FvSimpleFileSystemDxe.zip > > is legitimate prior to opening. To make sure this message is not > > infected with a virus, it is important to verify that you are > > expecting the message or else confirm its legitimacy with the sender. > > ------------------------------------------------------------------------------ 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
