Jordan, As Contributed-under and Signed-off-by message are mandatory infos, I didn't mention it.
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. For these series patches submitted by Martin, only this one gets approval at this time. Others need more discussion. Thanks Feng -----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 ------------------------------------------------------------------------------ Comprehensive Server Monitoring with Site24x7. Monitor 10 servers for $9/Month. Get alerted through email, SMS, voice calls or mobile push notifications. Take corrective actions from your mobile device. http://pubads.g.doubleclick.net/gampad/clk?id=154624111&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
