On Tue, May 21, 2013 at 10:39:57AM +0200, Laszlo Ersek wrote:
> Gary,
>
> (1) can you please repost this with a "more targeted" subject,
> identifying the affected subsystem(s)?
>
> Actually I think this should be split into a two-part series, one patch
> for EdkCompatibilityPkg and another for MdePkg:
>
> [PATCH v2 01/02] EdkCompatibilityPkg/BasePrintLib: add missing status
> strings for %r
> [PATCH v2 02/02] MdePkg/BasePrintLib: add missing status strings for %r
>
> (2) Also in each commit message please say something like
>
> Without this fix, the "%r" format specifier prints eg. "0000001A"
> instead of "Security Violation" for EFI_SECURITY_VIOLATION.
>
Thanks for the feedback.
> (3) Further 2*2 comments below:
>
>
> On 05/14/13 10:40, Gary Ching-Pang Lin wrote:
> >
> > Signed-off-by: Gary Ching-Pang Lin <g...@suse.com>
> > ---
> > .../Library/EdkIIGlueLib/Library/BasePrintLib/PrintLib.c | 11
> > +++++++++--
> > MdePkg/Library/BasePrintLib/PrintLibInternal.c | 11
> > +++++++++--
> > 2 files changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git
> > a/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePrintLib/PrintLib.c
> >
> > b/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePrintLib/PrintLib.c
> > index 3258b01..caddb84 100644
> > ---
> > a/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePrintLib/PrintLib.c
> > +++
> > b/EdkCompatibilityPkg/Foundation/Library/EdkIIGlueLib/Library/BasePrintLib/PrintLib.c
> > @@ -23,7 +23,7 @@ Abstract:
> > #include "PrintLibInternal.h"
> >
> > #define WARNING_STATUS_NUMBER 4
> > -#define ERROR_STATUS_NUMBER 24
> > +#define ERROR_STATUS_NUMBER 31
> > #define ASSERT_UNICODE_BUFFER(Buffer) ASSERT ((((UINTN) (Buffer)) & 0x01)
> > == 0)
> >
> > GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *StatusString [] = {
> > @@ -55,7 +55,14 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *StatusString
> > [] = {
> > "Aborted", // RETURN_ABORTED = 21 |
> > MAX_BIT
> > "ICMP Error", // RETURN_ICMP_ERROR = 22 |
> > MAX_BIT
> > "TFTP Error", // RETURN_TFTP_ERROR = 23 |
> > MAX_BIT
> > - "Protocol Error" // RETURN_PROTOCOL_ERROR = 24 |
> > MAX_BIT
> > + "Protocol Error", // RETURN_PROTOCOL_ERROR = 24 |
> > MAX_BIT
> > + "Incomplete Version", // RETURN_INCOMPATIBLE_VERSION = 25 |
> > MAX_BIT
>
> This is a typo; not "Incomplete" but "Incompatible".
>
Oops!
>
> > + "Security Violation", // RETURN_SECURITY_VIOLATION = 26 |
> > MAX_BIT
> > + "CRC Error", // RETURN_CRC_ERROR = 27 |
> > MAX_BIT
> > + "End of Media", // RETURN_END_OF_MEDIA = 28 |
> > MAX_BIT
> > + "", // RESERVED = 29 |
> > MAX_BIT
> > + "", // RESERVED = 30 |
> > MAX_BIT
>
> I think these last two should rather say "Reserved (29)" and "Reserved
> (30)".
>
OK
>
> > + "End of File" // RETURN_END_OF_FILE = 31 |
> > MAX_BIT
> > };
> >
> > /**
> > diff --git a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > index 34e9d12..b991a15 100644
> > --- a/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > +++ b/MdePkg/Library/BasePrintLib/PrintLibInternal.c
> > @@ -15,7 +15,7 @@
> > #include "PrintLibInternal.h"
> >
> > #define WARNING_STATUS_NUMBER 4
> > -#define ERROR_STATUS_NUMBER 24
> > +#define ERROR_STATUS_NUMBER 31
> >
> > GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 mHexStr[] =
> > {'0','1','2','3','4','5','6','7','8','9','A','B','C','D','E','F'};
> >
> > @@ -48,7 +48,14 @@ GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8
> > *mStatusString[] = {
> > "Aborted", // RETURN_ABORTED = 21 |
> > MAX_BIT
> > "ICMP Error", // RETURN_ICMP_ERROR = 22 |
> > MAX_BIT
> > "TFTP Error", // RETURN_TFTP_ERROR = 23 |
> > MAX_BIT
> > - "Protocol Error" // RETURN_PROTOCOL_ERROR = 24 |
> > MAX_BIT
> > + "Protocol Error", // RETURN_PROTOCOL_ERROR = 24 |
> > MAX_BIT
> > + "Incomplete Version", // RETURN_INCOMPATIBLE_VERSION = 25 |
> > MAX_BIT
> > + "Security Violation", // RETURN_SECURITY_VIOLATION = 26 |
> > MAX_BIT
> > + "CRC Error", // RETURN_CRC_ERROR = 27 |
> > MAX_BIT
> > + "End of Media", // RETURN_END_OF_MEDIA = 28 |
> > MAX_BIT
> > + "", // RESERVED = 29 |
> > MAX_BIT
> > + "", // RESERVED = 30 |
> > MAX_BIT
> > + "End of File" // RETURN_END_OF_FILE = 31 |
> > MAX_BIT
> > };
>
> Same two comments here.
>
>
> (4) Very important (maintainers won't consider your patch without this):
> add the following line (without the leading spaces) right before your
> "Signed-off-by":
>
> Contributed-under: TianoCore Contribution Agreement 1.0
>
> See "EdkCompatibilityPkg/Contributions.txt" and
> "MdePkg/Contributions.txt".
>
Arrgh, I should read them before sending the patch.
>
> I think this is a useful patch, please repost and get it merged! :)
>
Thanks! I'll refresh the patch and send it later.
Gary Lin
------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
