Re: [edk2] [PATCH] Add missing status strings.

Subject: Re: [edk2] [PATCH] Add missing status strings.

From: Gary Ching-Pang Lin <glin@suse.com>

To: Laszlo Ersek <lersek@redhat.com>

Date: 2013-05-22 02:21:05

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