Scott, I know that STATIC has also been heavily discussed about shell code.
I believe that the use of STATIC makes the code both more readable and safer as the compiler protects from certain behaviors. I don't know much about comdat folding, but do you have a patch to fix that root cause of this issue? -Jaben > -----Original Message----- > From: Scott Duplichan [mailto:sc...@notabs.org] > Sent: Friday, December 19, 2014 5:47 AM > To: edk2-devel@lists.sourceforge.net; 'Olivier Martin'; Justen, Jordan L; Jin, > Eric > Subject: Re: [edk2] patch review feedback about FvSimpleFileSystem > module > > Tian, Feng [mailto:feng.t...@intel.com] wrote: > > ]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). > > I appreciate Martin's use of STATIC for local functions. The recently updated > EDK2 coding document mentions not using static functions as a work-around > for a broken debugger. The broken debugger has never been identified. The > reason is clear: The problematic debugger behavior described is what > happens when comdat folding is enabled for the Microsoft tool chain. The > Microsoft tool chain was the only supported tool chain when this problem > was seen. EDK2 enables Microsoft comdat folding for all builds: RELEASE, > DEBUG, and even NOOPT. Clearly someone didn't realize comdat folding was > enabled and incorrectly assumed the resulting behavior was due to a > debugger bug. The correct solution to the problem is to either disable > Microsoft comdat folding for the NOOPT build or to recognize and live with > comdat folding. Banning an important part of the C language to work around > a problem that cannot be reproduced is not a sound engineering practice. > > Thanks, > Scott > > ]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.cl > ktrk > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel > > > ------------------------------------------------------------------------------ > 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.cl > ktrk > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ 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
