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
