public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: <Jim.Dailey@dell.com>
To: <jaben.carsey@intel.com>
Cc: <ruiyu.ni@intel.com>, <edk2-devel@lists.01.org>, <jeff.westfahl@ni.com>
Subject: Re: [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage
Date: Fri, 17 Feb 2017 17:35:57 +0000	[thread overview]
Message-ID: <c0a342b031834dae89bed50924c11baf@ausx13mps335.AMER.DELL.COM> (raw)
In-Reply-To: <CB6E33457884FA40993F35157061515C54B761FF@FMSMSX103.amr.corp.intel.com>

My point was that printing one byte of the data as hex is not much better (as far
as avoiding the crash is concerned) than simply not printing the data at all.  If
we are just trying to avoid the crash, printing nothing is even simpler.

If the correct representation is hex bytes, fine; but why print only 1 of them
and not all of them?


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Carsey, Jaben
Sent: Friday, February 17, 2017 11:28 AM
To: Jeff Westfahl <jeff.westfahl@ni.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage

I was reading the email.  I was also waiting and making sure there was consensus since I didn't have a strong opinion.  I will let Ray check also, but I think the fix is good.

Reviewed-by: Jaben Carsey <jaben.carsey@intel.com>

> -----Original Message-----
> From: Jeff Westfahl [mailto:jeff.westfahl@ni.com]
> Sent: Friday, February 17, 2017 8:55 AM
> To: Jeff Westfahl <jeff.westfahl@ni.com>
> Cc: edk2-devel@lists.01.org; Carsey, Jaben <jaben.carsey@intel.com>; Ni,
> Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format
> specifier for LoadedImage
> Importance: High
> 
> Jaben, Ruiyu,
> 
> Sorry, I forgot to cc you as the ShellPkg maintainers when I submitted
> this patch.
> 
> Regards,
> Jeff Westfahl
> 
> On Wed, 15 Feb 2017, Jeff Westfahl wrote:
> 
> > Jim,
> >
> > I agree that those are good ideas. However, such an implementation would
> > still crash on a BIOS built against the EDK II before commit 891d844. I think
> > it might be best to resolve the crash with the simple patch I have made,
> and
> > defer your suggestions for now.
> >
> > Regards,
> > Jeff
> >
> > On Tue, 14 Feb 2017, Jim.Dailey@dell.com wrote:
> >
> >> Please disregard the earlier "Confidential" text. The stupid plug-in that
> >> adds this
> >> does not show the text in the mail when it is composed in text mode, so I
> >> often
> >> forget to turn this "feature" off when posting.  Sorry.
> >>
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> >> Dailey, Jim
> >> Sent: Tuesday, February 14, 2017 4:40 PM
> >> To: jeff.westfahl@ni.com; edk2-devel@lists.01.org
> >> Subject: Re: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format
> >> specifier for LoadedImage
> >>
> >> Jeff,
> >>
> >> Perhaps a better approach is to print *all* the LoadOptions data as hex
> >> bytes?
> >>
> >> In addition, one might first analyze the LoadOptions data, and, when
> >> apropos,
> >> print obvious strings as strings?
> >>
> >> Regards,
> >> Jim
> >>
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jeff
> >> Westfahl
> >> Sent: Tuesday, February 14, 2017 3:54 PM
> >> To: edk2-devel@lists.01.org
> >> Subject: [edk2] [PATCH] ShellPkg/HandleParsingLib: Correct format
> specifier
> >> for LoadedImage
> >>
> >> The format specifier for the LoadOptions field of the LoadedImage
> protocol
> >> is "%s". However, the data in LoadOptions is often generic binary data. A
> >> format specifier of "%x" is more appropriate for this field.
> >>
> >> Using "dh -v" with format specifier "%s" on BIOS images based on EDK II
> >> source before commit 891d844 can cause a crash.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Jeff Westfahl <jeff.westfahl@ni.com>
> >> ---
> >> ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git
> a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> index 0d51627c5f..273a4201bc 100644
> >> --- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> +++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.uni
> >> @@ -354,7 +354,7 @@
> >>                                                   "     DeviceHandle..:
> >> %%H%x%%N\r\n"
> >>                                                   "     FilePath......:
> >> %%H%x%%N\r\n"
> >>                                                   "     OptionsSize...:
> >> %%H%x%%N\r\n"
> >> -                                                  "     LoadOptions...:
> >> %%H%s%%N\r\n"
> >> +                                                  "     LoadOptions...:
> >> %%H%x%%N\r\n"
> >>                                                   "     ImageBase.....:
> >> %%H%x%%N\r\n"
> >>                                                   "     ImageSize.....:
> >> %%H%Lx%%N\r\n"
> >>                                                   "     CodeType......:
> >> %%H%s%%N\r\n"
> >> --
> >> 2.11.0.windows.3
> >>
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> >>
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


  reply	other threads:[~2017-02-17 17:36 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-14 21:54 [PATCH] ShellPkg/HandleParsingLib: Correct format specifier for LoadedImage Jeff Westfahl
2017-02-14 22:40 ` Jim.Dailey
2017-02-14 22:44   ` Jim.Dailey
2017-02-15 20:41     ` Jeff Westfahl
2017-02-17 16:54       ` Jeff Westfahl
2017-02-17 17:27         ` Carsey, Jaben
2017-02-17 17:35           ` Jim.Dailey [this message]
2017-02-17 17:37             ` Carsey, Jaben
2017-02-17 17:43               ` Jeff Westfahl
2017-02-17 18:12                 ` Jim.Dailey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c0a342b031834dae89bed50924c11baf@ausx13mps335.AMER.DELL.COM \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox