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 ------------------------------------------------------------------------------ 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
