Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-18 Thread Wu, Jiaxin 
Got it, the patch will be sent out later. 

Thanks.
Jiaxin

> -Original Message-
> From: Carsey, Jaben
> Sent: Tuesday, April 19, 2016 1:41 AM
> To: Wu, Jiaxin ; David Van Arnem
> ; Bhupesh Sharma ;
> Laszlo Ersek ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan ; Carsey,
> Jaben 
> Subject: RE: [edk2] [Patch] ShellPkg: Update ping command options to sync
> with Spec
> 
> That sounds like a good enhancement to me.
> 
> -Jaben
> 
> 
> > -Original Message-
> > From: Wu, Jiaxin
> > Sent: Thursday, April 14, 2016 8:52 PM
> > To: Carsey, Jaben ; David Van Arnem
> > ; Bhupesh Sharma ;
> Laszlo
> > Ersek ; edk2-devel@lists.01.org
> > Cc: Ye, Ting ; Fu, Siyuan 
> > Subject: RE: [edk2] [Patch] ShellPkg: Update ping command options to
> > sync with Spec
> > Importance: High
> >
> > Jaben ,
> >
> > Do you agree to support no source IP specified case? After update,
> > Ping will select the first both connected and configured interface
> > automatically instead of always the first NIC.
> > If no objection, patch will be sent out for review.
> >
> > Thanks.
> > Jiaxin
> >
> >
> > > -Original Message-
> > > From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> > > Sent: Friday, April 15, 2016 1:32 AM
> > > To: Wu, Jiaxin ; Bhupesh Sharma
> > > ; Laszlo Ersek ; edk2-
> > > de...@lists.01.org
> > > Cc: Ye, Ting ; Carsey, Jaben
> > > ; Fu, Siyuan 
> > > Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to
> > > sync with Spec
> > >
> > > On 04/13/2016 09:13 PM, Wu, Jiaxin wrote:
> > > > Hi David and Laszlo,
> > > >
> > > > This patch is not focused on the discussion about the multiple
> > > > NICs existed
> > > case, just use to update the ping command option. So, please don't
> > > misunderstand.
> > > >
> > >
> > > Ah, ok.  Sorry to clutter up this patch discussion then, I wanted to
> > > make sure the multiple NIC case wasn't lost in the discussion about "-
> s"/"-_s".  Thanks!
> > >
> > > > For the discussion in
> > > <http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>, If there
> > > are multiple NICs existed in the platform and no source IP is specified
> (no '-s'
> > > option), the first NIC will be selected as default interface to ping
> > > the target. '- s SourceIp' can be used to specify any interface you
> > > want. That's the current implementation choice.
> > > >
> > > > I agree your and Bhupesh 's option to support no source IP
> > > > specified case,
> > > that's meaningful to improve the command's friendliness(Windows and
> > Linus
> > > do this resolution). How about selecting the first both connected and
> > > configured interface automatically?   If so, I can create another patch to
> > > support that solution.
> > > >
> > >
> > > That sounds great to me, as long as it's not too much trouble :-)
> > > Alternately,
> > a
> > > more useful error message in that case might suffice (e.g. "Multiple
> > > configured interfaces found, please specify one with '-s'").  But,
> > > since Windows and Linux both do the automatic resolution, I think
> > > consistency would be beneficial.
> > >
> > > Thanks again!
> > >
> > > David
> > >
> > > > Thanks.
> > > > Jiaxin
> > > >
> > > >> -Original Message-
> > > >> From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> > > >> Sent: Thursday, April 14, 2016 4:02 AM
> > > >> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> > > >> Cc: Ye, Ting ; Carsey, Jaben
> > > >> ; Fu, Siyuan 
> > > >> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options
> > > >> to sync with Spec
> > > >>
> > > >> On 04/12/2016 09:16 PM, Jiaxin Wu wrote:
> > > >>> This patch is used to update ping command options to sync with
> > > >>> shell2.2 Spec.
> > > >>> Considering the backward compatible issue, the patch keeps ‘-_s’
> > > >>> command option unchanged, only add the new option '-s'
> > > >>> and make the old option '-_s' function same as new one.
> > > >&g

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-18 Thread Carsey, Jaben 
That sounds like a good enhancement to me.

-Jaben


> -Original Message-
> From: Wu, Jiaxin
> Sent: Thursday, April 14, 2016 8:52 PM
> To: Carsey, Jaben ; David Van Arnem
> ; Bhupesh Sharma ;
> Laszlo Ersek ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan 
> Subject: RE: [edk2] [Patch] ShellPkg: Update ping command options to sync
> with Spec
> Importance: High
> 
> Jaben ,
> 
> Do you agree to support no source IP specified case? After update, Ping will
> select the first both connected and configured interface automatically instead
> of always the first NIC.
> If no objection, patch will be sent out for review.
> 
> Thanks.
> Jiaxin
> 
> 
> > -Original Message-
> > From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> > Sent: Friday, April 15, 2016 1:32 AM
> > To: Wu, Jiaxin ; Bhupesh Sharma
> > ; Laszlo Ersek ; edk2-
> > de...@lists.01.org
> > Cc: Ye, Ting ; Carsey, Jaben ;
> > Fu, Siyuan 
> > Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to sync
> > with Spec
> >
> > On 04/13/2016 09:13 PM, Wu, Jiaxin wrote:
> > > Hi David and Laszlo,
> > >
> > > This patch is not focused on the discussion about the multiple NICs 
> > > existed
> > case, just use to update the ping command option. So, please don't
> > misunderstand.
> > >
> >
> > Ah, ok.  Sorry to clutter up this patch discussion then, I wanted to make 
> > sure
> > the multiple NIC case wasn't lost in the discussion about "-s"/"-_s".  
> > Thanks!
> >
> > > For the discussion in
> > <http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>, If there
> > are multiple NICs existed in the platform and no source IP is specified (no 
> > '-s'
> > option), the first NIC will be selected as default interface to ping the 
> > target. '-
> > s SourceIp' can be used to specify any interface you want. That's the 
> > current
> > implementation choice.
> > >
> > > I agree your and Bhupesh 's option to support no source IP specified case,
> > that's meaningful to improve the command's friendliness(Windows and
> Linus
> > do this resolution). How about selecting the first both connected and
> > configured interface automatically?   If so, I can create another patch to
> > support that solution.
> > >
> >
> > That sounds great to me, as long as it's not too much trouble :-) 
> > Alternately,
> a
> > more useful error message in that case might suffice (e.g. "Multiple
> > configured interfaces found, please specify one with '-s'").  But, since
> > Windows and Linux both do the automatic resolution, I think consistency
> > would be beneficial.
> >
> > Thanks again!
> >
> > David
> >
> > > Thanks.
> > > Jiaxin
> > >
> > >> -Original Message-
> > >> From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> > >> Sent: Thursday, April 14, 2016 4:02 AM
> > >> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> > >> Cc: Ye, Ting ; Carsey, Jaben
> > >> ; Fu, Siyuan 
> > >> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to
> > >> sync with Spec
> > >>
> > >> On 04/12/2016 09:16 PM, Jiaxin Wu wrote:
> > >>> This patch is used to update ping command options to sync with
> > >>> shell2.2 Spec.
> > >>> Considering the backward compatible issue, the patch keeps ‘-_s’
> > >>> command option unchanged, only add the new option '-s'
> > >>> and make the old option '-_s' function same as new one.
> > >>>
> > >>> Cc: Ye Ting 
> > >>> Cc: Fu Siyuan 
> > >>> Cc: Jaben Carsey 
> > >>> Contributed-under: TianoCore Contribution Agreement 1.0
> > >>> Signed-off-by: Jiaxin Wu 
> > >>> ---
> > >>>.../Library/UefiShellNetwork1CommandsLib/Ping.c| 12
> ++-
> > -
> > >>>.../UefiShellNetwork1CommandsLib.uni   | 22 
> > >>> +
> -
> > 
> > >>>2 files changed, 19 insertions(+), 15 deletions(-)
> > >>>
> > >>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > >>> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > >>> index dbee764..13bcdde

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-14 Thread Wu, Jiaxin 
Jaben ,

Do you agree to support no source IP specified case? After update, Ping will 
select the first both connected and configured interface automatically instead 
of always the first NIC.
If no objection, patch will be sent out for review.

Thanks.
Jiaxin


> -Original Message-
> From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> Sent: Friday, April 15, 2016 1:32 AM
> To: Wu, Jiaxin ; Bhupesh Sharma
> ; Laszlo Ersek ; edk2-
> de...@lists.01.org
> Cc: Ye, Ting ; Carsey, Jaben ;
> Fu, Siyuan 
> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to sync
> with Spec
> 
> On 04/13/2016 09:13 PM, Wu, Jiaxin wrote:
> > Hi David and Laszlo,
> >
> > This patch is not focused on the discussion about the multiple NICs existed
> case, just use to update the ping command option. So, please don't
> misunderstand.
> >
> 
> Ah, ok.  Sorry to clutter up this patch discussion then, I wanted to make sure
> the multiple NIC case wasn't lost in the discussion about "-s"/"-_s".  Thanks!
> 
> > For the discussion in
> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>, If there
> are multiple NICs existed in the platform and no source IP is specified (no 
> '-s'
> option), the first NIC will be selected as default interface to ping the 
> target. '-
> s SourceIp' can be used to specify any interface you want. That's the current
> implementation choice.
> >
> > I agree your and Bhupesh 's option to support no source IP specified case,
> that's meaningful to improve the command's friendliness(Windows and Linus
> do this resolution). How about selecting the first both connected and
> configured interface automatically?   If so, I can create another patch to
> support that solution.
> >
> 
> That sounds great to me, as long as it's not too much trouble :-) 
> Alternately, a
> more useful error message in that case might suffice (e.g. "Multiple
> configured interfaces found, please specify one with '-s'").  But, since
> Windows and Linux both do the automatic resolution, I think consistency
> would be beneficial.
> 
> Thanks again!
> 
> David
> 
> > Thanks.
> > Jiaxin
> >
> >> -Original Message-
> >> From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> >> Sent: Thursday, April 14, 2016 4:02 AM
> >> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> >> Cc: Ye, Ting ; Carsey, Jaben
> >> ; Fu, Siyuan 
> >> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to
> >> sync with Spec
> >>
> >> On 04/12/2016 09:16 PM, Jiaxin Wu wrote:
> >>> This patch is used to update ping command options to sync with
> >>> shell2.2 Spec.
> >>> Considering the backward compatible issue, the patch keeps ‘-_s’
> >>> command option unchanged, only add the new option '-s'
> >>> and make the old option '-_s' function same as new one.
> >>>
> >>> Cc: Ye Ting 
> >>> Cc: Fu Siyuan 
> >>> Cc: Jaben Carsey 
> >>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>> Signed-off-by: Jiaxin Wu 
> >>> ---
> >>>.../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++-
> -
> >>>.../UefiShellNetwork1CommandsLib.uni   | 22 
> >>> +-
> 
> >>>2 files changed, 19 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >>> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >>> index dbee764..13bcdde 100644
> >>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> >>> @@ -1,10 +1,10 @@
> >>>/** @file
> >>>  The implementation for Ping shell command.
> >>>
> >>>  (C) Copyright 2015 Hewlett-Packard Development Company,
> >>> L.P.
> >>> -  Copyright (c) 2009 - 2015, Intel Corporation. All rights
> >>> reserved.
> >>> +  Copyright (c) 2009 - 2016, Intel Corporation. All rights
> >>> + reserved.
> >>>
> >>>  This program and the accompanying materials
> >>>  are licensed and made available under the terms and conditions
> >>> of the
> >> BSD License
> >>>  which accompanies this distribution.  The full text of the
> >>> license may be
> >> found at
&g

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-14 Thread David Van Arnem 

On 04/13/2016 09:13 PM, Wu, Jiaxin wrote:

Hi David and Laszlo,

This patch is not focused on the discussion about the multiple NICs existed 
case, just use to update the ping command option. So, please don't 
misunderstand.



Ah, ok.  Sorry to clutter up this patch discussion then, I wanted to 
make sure the multiple NIC case wasn't lost in the discussion about 
"-s"/"-_s".  Thanks!



For the discussion in 
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>, If there are 
multiple NICs existed in the platform and no source IP is specified (no '-s' option), 
the first NIC will be selected as default interface to ping the target. '-s SourceIp' 
can be used to specify any interface you want. That's the current implementation 
choice.

I agree your and Bhupesh 's option to support no source IP specified case, 
that's meaningful to improve the command's friendliness(Windows and Linus do 
this resolution). How about selecting the first both connected and configured 
interface automatically?   If so, I can create another patch to support that 
solution.



That sounds great to me, as long as it's not too much trouble :-) 
Alternately, a more useful error message in that case might suffice 
(e.g. "Multiple configured interfaces found, please specify one with 
'-s'").  But, since Windows and Linux both do the automatic resolution, 
I think consistency would be beneficial.


Thanks again!

David


Thanks.
Jiaxin


-Original Message-
From: David Van Arnem [mailto:dvanar...@cmlab.biz]
Sent: Thursday, April 14, 2016 4:02 AM
To: Wu, Jiaxin ; edk2-devel@lists.01.org
Cc: Ye, Ting ; Carsey, Jaben ;
Fu, Siyuan 
Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to sync
with Spec

On 04/12/2016 09:16 PM, Jiaxin Wu wrote:

This patch is used to update ping command options to sync with
shell2.2 Spec.
Considering the backward compatible issue, the patch keeps ‘-_s’
command option unchanged, only add the new option '-s'
and make the old option '-_s' function same as new one.

Cc: Ye Ting 
Cc: Fu Siyuan 
Cc: Jaben Carsey 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
   .../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++--
   .../UefiShellNetwork1CommandsLib.uni   | 22 
+-
   2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index dbee764..13bcdde 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -1,10 +1,10 @@
   /** @file
 The implementation for Ping shell command.

 (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
-  Copyright (c) 2009 - 2015, Intel Corporation. All rights
reserved.
+  Copyright (c) 2009 - 2016, Intel Corporation. All rights
+ reserved.

 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the

BSD License

 which accompanies this distribution.  The full text of the license may be

found at

 http://opensource.org/licenses/bsd-license.php.
@@ -196,10 +196,14 @@ STATIC CONST SHELL_PARAM_ITEM

PingParamList[] = {

 {
   L"-n",
   TypeValue
 },
 {
+L"-s",
+TypeValue
+  },
+  {
   L"-_s",
   TypeValue
 },
 {
   L"-_ip6",
@@ -1510,11 +1514,15 @@ ShellCommandRunPing (
 ZeroMem (&DstAddress, sizeof (EFI_IPv6_ADDRESS));

 //
 // Parse the paramter of source ip address.
 //
-  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
+  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-s");  if
+ (ValueStr == NULL) {
+ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");  }
+


What happens to ValueStr when neither "-s" nor "-_s" are specified from the
command line, but there are multiple network interfaces active on the
system?  Will ValueStr be resolved to one of their IP's automatically?

In other words, if the system has two active network interfaces, will ping
work with just the command:

ping 

(I would test here but I do not have a platform that will work, unfortunately).

I think this was (part of) Bhupesh's original issue (below from
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>):

  > We are facing an issue when we enable two SNP (Simple Network) drivers
in our EDK2 setup over a ARMv8 based  > SoC - NXP LS2080A.
  >
  > 
  >
  > Shell> ifconfig -l
  >
  > -
  >
  > name : eth0
  > Media State  : Media pres

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-14 Thread Laszlo Ersek 
On 04/14/16 05:13, Wu, Jiaxin wrote:
> Hi David and Laszlo,
> 
> This patch is not focused on the discussion about the multiple NICs
> existed case, just use to update the ping command option. So, please
> don't misunderstand.

Ah, okay. Although this patch is worthwhile without any particular
reasoning too (of course), I associated it with that other discussion,
and I thought your motivation was Bhupesh's use case.

If your "only" goal was to improve the shell's conformance to the spec,
that makes the patch 100% justified as well, naturally. And it just
happens to fix Bhupesh's use case on the side.

(I still think that it would be nice to hear back from Bhupesh.)

> For the discussion in
> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>, If there
> are multiple NICs existed in the platform and no source IP is
> specified (no '-s' option), the first NIC will be selected as default
> interface to ping the target. '-s SourceIp' can be used to specify
> any interface you want. That's the current implementation choice.
> 
> I agree your and Bhupesh 's option to support no source IP specified
> case, that's meaningful to improve the command's friendliness(Windows
> and Linus do this resolution). How about selecting the first both
> connected and configured interface automatically? If so, I can create
> another patch to support that solution.

That's very kind of you, thank you -- I personally didn't mean to submit
a new feature request :) I'm neutral on the question whether such an
automatism should be added to the ping command, in the UEFI shell.

Thank you!
Laszlo

> Thanks.
> Jiaxin
> 
>> -Original Message-
>> From: David Van Arnem [mailto:dvanar...@cmlab.biz]
>> Sent: Thursday, April 14, 2016 4:02 AM
>> To: Wu, Jiaxin ; edk2-devel@lists.01.org
>> Cc: Ye, Ting ; Carsey, Jaben ;
>> Fu, Siyuan 
>> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to sync
>> with Spec
>>
>> On 04/12/2016 09:16 PM, Jiaxin Wu wrote:
>>> This patch is used to update ping command options to sync with
>>> shell2.2 Spec.
>>> Considering the backward compatible issue, the patch keeps ‘-_s’
>>> command option unchanged, only add the new option '-s'
>>> and make the old option '-_s' function same as new one.
>>>
>>> Cc: Ye Ting 
>>> Cc: Fu Siyuan 
>>> Cc: Jaben Carsey 
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Jiaxin Wu 
>>> ---
>>>   .../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++--
>>>   .../UefiShellNetwork1CommandsLib.uni   | 22 
>>> +-
>>>   2 files changed, 19 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>>> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>>> index dbee764..13bcdde 100644
>>> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>>> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
>>> @@ -1,10 +1,10 @@
>>>   /** @file
>>> The implementation for Ping shell command.
>>>
>>> (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
>>> -  Copyright (c) 2009 - 2015, Intel Corporation. All rights
>>> reserved.
>>> +  Copyright (c) 2009 - 2016, Intel Corporation. All rights
>>> + reserved.
>>>
>>> This program and the accompanying materials
>>> are licensed and made available under the terms and conditions of the
>> BSD License
>>> which accompanies this distribution.  The full text of the license may 
>>> be
>> found at
>>> http://opensource.org/licenses/bsd-license.php.
>>> @@ -196,10 +196,14 @@ STATIC CONST SHELL_PARAM_ITEM
>> PingParamList[] = {
>>> {
>>>   L"-n",
>>>   TypeValue
>>> },
>>> {
>>> +L"-s",
>>> +TypeValue
>>> +  },
>>> +  {
>>>   L"-_s",
>>>   TypeValue
>>> },
>>> {
>>>   L"-_ip6",
>>> @@ -1510,11 +1514,15 @@ ShellCommandRunPing (
>>> ZeroMem (&DstAddress, sizeof (EFI_IPv6_ADDRESS));
>>>
>>> //
>>> // Parse the paramter of source ip address.
>>> //
>>> -  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
>>> +  ValueStr = ShellCommandLineGetValue (Param

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-13 Thread Wu, Jiaxin 
Hi David and Laszlo,

This patch is not focused on the discussion about the multiple NICs existed 
case, just use to update the ping command option. So, please don't 
misunderstand. 

For the discussion in 
<http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>, If there are 
multiple NICs existed in the platform and no source IP is specified (no '-s' 
option), the first NIC will be selected as default interface to ping the 
target. '-s SourceIp' can be used to specify any interface you want. That's the 
current implementation choice. 

I agree your and Bhupesh 's option to support no source IP specified case, 
that's meaningful to improve the command's friendliness(Windows and Linus do 
this resolution). How about selecting the first both connected and configured 
interface automatically? If so, I can create another patch to support that 
solution. 

Thanks.
Jiaxin

> -Original Message-
> From: David Van Arnem [mailto:dvanar...@cmlab.biz]
> Sent: Thursday, April 14, 2016 4:02 AM
> To: Wu, Jiaxin ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Carsey, Jaben ;
> Fu, Siyuan 
> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to sync
> with Spec
> 
> On 04/12/2016 09:16 PM, Jiaxin Wu wrote:
> > This patch is used to update ping command options to sync with
> > shell2.2 Spec.
> > Considering the backward compatible issue, the patch keeps ‘-_s’
> > command option unchanged, only add the new option '-s'
> > and make the old option '-_s' function same as new one.
> >
> > Cc: Ye Ting 
> > Cc: Fu Siyuan 
> > Cc: Jaben Carsey 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu 
> > ---
> >   .../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++--
> >   .../UefiShellNetwork1CommandsLib.uni   | 22 
> > +-
> >   2 files changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > index dbee764..13bcdde 100644
> > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > @@ -1,10 +1,10 @@
> >   /** @file
> > The implementation for Ping shell command.
> >
> > (C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> > -  Copyright (c) 2009 - 2015, Intel Corporation. All rights
> > reserved.
> > +  Copyright (c) 2009 - 2016, Intel Corporation. All rights
> > + reserved.
> >
> > This program and the accompanying materials
> > are licensed and made available under the terms and conditions of the
> BSD License
> > which accompanies this distribution.  The full text of the license may 
> > be
> found at
> > http://opensource.org/licenses/bsd-license.php.
> > @@ -196,10 +196,14 @@ STATIC CONST SHELL_PARAM_ITEM
> PingParamList[] = {
> > {
> >   L"-n",
> >   TypeValue
> > },
> > {
> > +L"-s",
> > +TypeValue
> > +  },
> > +  {
> >   L"-_s",
> >   TypeValue
> > },
> > {
> >   L"-_ip6",
> > @@ -1510,11 +1514,15 @@ ShellCommandRunPing (
> > ZeroMem (&DstAddress, sizeof (EFI_IPv6_ADDRESS));
> >
> > //
> > // Parse the paramter of source ip address.
> > //
> > -  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
> > +  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-s");  if
> > + (ValueStr == NULL) {
> > +ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");  }
> > +
> 
> What happens to ValueStr when neither "-s" nor "-_s" are specified from the
> command line, but there are multiple network interfaces active on the
> system?  Will ValueStr be resolved to one of their IP's automatically?
> 
> In other words, if the system has two active network interfaces, will ping
> work with just the command:
> 
> ping 
> 
> (I would test here but I do not have a platform that will work, 
> unfortunately).
> 
> I think this was (part of) Bhupesh's original issue (below from
> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>):
> 
>  > We are facing an issue when we enable two SNP (Simple Network) drivers
> in our EDK2 setup over a ARMv8 based  > SoC - NXP LS2080A.
>  >
>  > 
>  >
>  > Shell> ifconfig -l
>  >
>  > ---

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-13 Thread David Van Arnem 

On 04/12/2016 09:16 PM, Jiaxin Wu wrote:

This patch is used to update ping command options to sync
with shell2.2 Spec.
Considering the backward compatible issue, the patch keeps
‘-_s’ command option unchanged, only add the new option '-s'
and make the old option '-_s' function same as new one.

Cc: Ye Ting 
Cc: Fu Siyuan 
Cc: Jaben Carsey 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiaxin Wu 
---
  .../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++--
  .../UefiShellNetwork1CommandsLib.uni   | 22 +-
  2 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c 
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
index dbee764..13bcdde 100644
--- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
+++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
@@ -1,10 +1,10 @@
  /** @file
The implementation for Ping shell command.

(C) Copyright 2015 Hewlett-Packard Development Company, L.P.
-  Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.
+  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.

This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD 
License
which accompanies this distribution.  The full text of the license may be 
found at
http://opensource.org/licenses/bsd-license.php.
@@ -196,10 +196,14 @@ STATIC CONST SHELL_PARAM_ITEMPingParamList[] = {
{
  L"-n",
  TypeValue
},
{
+L"-s",
+TypeValue
+  },
+  {
  L"-_s",
  TypeValue
},
{
  L"-_ip6",
@@ -1510,11 +1514,15 @@ ShellCommandRunPing (
ZeroMem (&DstAddress, sizeof (EFI_IPv6_ADDRESS));

//
// Parse the paramter of source ip address.
//
-  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
+  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-s");
+  if (ValueStr == NULL) {
+ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
+  }
+


What happens to ValueStr when neither "-s" nor "-_s" are specified from 
the command line, but there are multiple network interfaces active on 
the system?  Will ValueStr be resolved to one of their IP's automatically?


In other words, if the system has two active network interfaces, will 
ping work with just the command:


ping 

(I would test here but I do not have a platform that will work, 
unfortunately).


I think this was (part of) Bhupesh's original issue (below from 
):


> We are facing an issue when we enable two SNP (Simple Network) 
drivers in our EDK2 setup over a ARMv8 based

> SoC - NXP LS2080A.
>
> 
>
> Shell> ifconfig -l
>
> -
>
> name : eth0
> Media State  : Media present
> policy   : dhcp
> mac addr : 68:05:CA:16:8C:5E
>
> 
> -
>
> name : eth1
> Media State  : Media present
> policy   : static
> mac addr : 6E:70:FE:EC:00:06
>
> 
> -
> C) Now, when I try to ping a server over a LAN cable, I get the 
following error message:

>
> Shell> ping 192.168.1.1
> InstallProtocolInterface: 41D94CD2-35B6-455A-8258-D4E51334AADD 8370DFD9E0
> InstallProtocolInterface: 8A219718-4EF5-4761-91C8-C0F04BDA9E56 8370DFF8A0
>
> Snp->undi.transmit()  8000h:Ah
> Config No mapping

Note that he did not specify an interface in the ping command, but the 
issue was resolved when he did (which started the "-_s"/"-s" 
discussion). In my experience, Linux's ping will choose an active 
interface when there are multiple active interfaces but the "-I" flag is 
unused. I don't know if automatic interface resolution is desired in the 
Shell's ping (perhaps Windows doesn't do this resolution), and if not 
that's fine, but I figure it's worth mentioning.


Thanks!

David


if (ValueStr != NULL) {
  mSrcString = ValueStr;
  if (IpChoice == PING_IP_CHOICE_IP6) {
Status = NetLibStrToIp6 (ValueStr, &SrcAddress);
  } else {
diff --git 
a/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
 
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
index bc6acac..7d6f2da 100644
--- 
a/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
+++ 
b/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
@@ -1,9 +1,9 @@
  // /**
  //
  // (C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P.
-// Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved. 
+// Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved. 
  // This program and the accompanying materials
  // are licensed and made available under the terms and conditions of the BSD 
License
  // which accompanies this

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-13 Thread Carsey, Jaben 
Patch looks good, but let's make sure to give Bhupesh some time to test.

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, April 13, 2016 3:30 AM
> To: Wu, Jiaxin ; Bhupesh Sharma
> 
> Cc: edk2-de...@ml01.01.org; Ye, Ting ; Carsey, Jaben
> ; Fu, Siyuan ; David Van
> Arnem 
> Subject: Re: [edk2] [Patch] ShellPkg: Update ping command options to sync
> with Spec
> Importance: High
> 
> On 04/13/16 05:16, Jiaxin Wu wrote:
> > This patch is used to update ping command options to sync
> > with shell2.2 Spec.
> > Considering the backward compatible issue, the patch keeps
> > ‘-_s’ command option unchanged, only add the new option '-s'
> > and make the old option '-_s' function same as new one.
> >
> > Cc: Ye Ting 
> > Cc: Fu Siyuan 
> > Cc: Jaben Carsey 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Jiaxin Wu 
> > ---
> >  .../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++--
> >  .../UefiShellNetwork1CommandsLib.uni   | 22 
> > +-
> >  2 files changed, 19 insertions(+), 15 deletions(-)
> 
> Bhupesh, can you apply this patch locally, and report back to Jiaxin
> with your test results?
> 
> Because, I think this patch was motivated by the discussion in
> <http://thread.gmane.org/gmane.comp.bios.edk2.devel/10046>.
> 
> Jiaxin, in such cases, it is best practice to:
> - add a "Reported-by:" tag to the patch, such as:
> 
>   Reported-by: Bhupesh Sharma 
> 
> - please consider also Cc:-ing the reporter (so he's aware of the patch
> and can respond with test results).
> 
> Just suggestions, of course.
> 
> Thanks!
> Laszlo
> 
> > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > index dbee764..13bcdde 100644
> > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> > @@ -1,10 +1,10 @@
> >  /** @file
> >The implementation for Ping shell command.
> >
> >(C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> > -  Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
> >
> >This program and the accompanying materials
> >are licensed and made available under the terms and conditions of the
> BSD License
> >which accompanies this distribution.  The full text of the license may be
> found at
> >http://opensource.org/licenses/bsd-license.php.
> > @@ -196,10 +196,14 @@ STATIC CONST SHELL_PARAM_ITEM
> PingParamList[] = {
> >{
> >  L"-n",
> >  TypeValue
> >},
> >{
> > +L"-s",
> > +TypeValue
> > +  },
> > +  {
> >  L"-_s",
> >  TypeValue
> >},
> >{
> >  L"-_ip6",
> > @@ -1510,11 +1514,15 @@ ShellCommandRunPing (
> >ZeroMem (&DstAddress, sizeof (EFI_IPv6_ADDRESS));
> >
> >//
> >// Parse the paramter of source ip address.
> >//
> > -  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
> > +  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-s");
> > +  if (ValueStr == NULL) {
> > +ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
> > +  }
> > +
> >if (ValueStr != NULL) {
> >  mSrcString = ValueStr;
> >  if (IpChoice == PING_IP_CHOICE_IP6) {
> >Status = NetLibStrToIp6 (ValueStr, &SrcAddress);
> >  } else {
> > diff --git
> a/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Comm
> andsLib.uni
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Comm
> andsLib.uni
> > index bc6acac..7d6f2da 100644
> > ---
> a/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Comm
> andsLib.uni
> > +++
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1Comm
> andsLib.uni
> > @@ -1,9 +1,9 @@
> >  // /**
> >  //
> >  // (C) Copyright 2013-2015 Hewlett-Packard Development Company,
> L.P.
> > -// Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved. 
> > +// Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved. 
> >  // This program and the accompanying materials
> >  // are licensed and made available under the terms and conditions of the
>

Re: [edk2] [Patch] ShellPkg: Update ping command options to sync with Spec

2016-04-13 Thread Laszlo Ersek 
On 04/13/16 05:16, Jiaxin Wu wrote:
> This patch is used to update ping command options to sync
> with shell2.2 Spec.
> Considering the backward compatible issue, the patch keeps
> ‘-_s’ command option unchanged, only add the new option '-s'
> and make the old option '-_s' function same as new one.
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Cc: Jaben Carsey 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiaxin Wu 
> ---
>  .../Library/UefiShellNetwork1CommandsLib/Ping.c| 12 ++--
>  .../UefiShellNetwork1CommandsLib.uni   | 22 
> +-
>  2 files changed, 19 insertions(+), 15 deletions(-)

Bhupesh, can you apply this patch locally, and report back to Jiaxin
with your test results?

Because, I think this patch was motivated by the discussion in
.

Jiaxin, in such cases, it is best practice to:
- add a "Reported-by:" tag to the patch, such as:

  Reported-by: Bhupesh Sharma 

- please consider also Cc:-ing the reporter (so he's aware of the patch
and can respond with test results).

Just suggestions, of course.

Thanks!
Laszlo

> diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c 
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> index dbee764..13bcdde 100644
> --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ping.c
> @@ -1,10 +1,10 @@
>  /** @file
>The implementation for Ping shell command.
>  
>(C) Copyright 2015 Hewlett-Packard Development Company, L.P.
> -  Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
>which accompanies this distribution.  The full text of the license may be 
> found at
>http://opensource.org/licenses/bsd-license.php.
> @@ -196,10 +196,14 @@ STATIC CONST SHELL_PARAM_ITEMPingParamList[] = {
>{
>  L"-n",
>  TypeValue
>},
>{
> +L"-s",
> +TypeValue
> +  },
> +  {
>  L"-_s",
>  TypeValue
>},
>{
>  L"-_ip6",
> @@ -1510,11 +1514,15 @@ ShellCommandRunPing (
>ZeroMem (&DstAddress, sizeof (EFI_IPv6_ADDRESS));
>  
>//
>// Parse the paramter of source ip address.
>//
> -  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
> +  ValueStr = ShellCommandLineGetValue (ParamPackage, L"-s");
> +  if (ValueStr == NULL) {
> +ValueStr = ShellCommandLineGetValue (ParamPackage, L"-_s");
> +  }
> +  
>if (ValueStr != NULL) {
>  mSrcString = ValueStr;
>  if (IpChoice == PING_IP_CHOICE_IP6) {
>Status = NetLibStrToIp6 (ValueStr, &SrcAddress);
>  } else {
> diff --git 
> a/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
>  
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
> index bc6acac..7d6f2da 100644
> --- 
> a/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
> +++ 
> b/ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.uni
> @@ -1,9 +1,9 @@
>  // /**
>  //
>  // (C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P.
> -// Copyright (c) 2010 - 2015, Intel Corporation. All rights reserved. 
> +// Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved. 
>  // This program and the accompanying materials
>  // are licensed and made available under the terms and conditions of the BSD 
> License
>  // which accompanies this distribution. The full text of the license may be 
> found at
>  // http://opensource.org/licenses/bsd-license.php
>  //
> @@ -86,28 +86,27 @@
>  #string STR_IFCONFIG_INFO_GATEWAY_HEAD#language en-US
> "\n%Hdefault gateway: %N"
>  #string STR_IFCONFIG_INFO_DNS_ADDR_HEAD   #language en-US"\n%HDNS 
> server   : %N\n"
>  #string STR_IFCONFIG_INFO_IP_ADDR_BODY#language en-US
> "%d.%d.%d.%d\n"
>  
>  #string STR_GET_HELP_PING #language en-US ""
> -".TH ping 0 "Pings the target host with an IPv4 or IPv6 stack."\r\n"
> +".TH ping 0 "Ping the target host with an IPv4 stack."\r\n"
>  ".SH NAME\r\n"
> -"Pings the target host with an IPv4 or IPv6 stack.\r\n"
> +"Ping the target host with an IPv4 stack.\r\n"
>  ".SH SYNOPSIS\r\n"
>  " \r\n"
> -"PING [-_ip6] [-_s SourceIp] [-n count] [-l size] TargetIp\r\n"
> +"PING [-n count] [-l size] [-s SourceIp] TargetIp\r\n"
>  ".SH OPTIONS\r\n"
>  " \r\n"
>  "  -n   - Specifies the number of echo request datagrams to be sent.\r\n"
>  "  -l   - Specifies the size of the data buffer in the echo request 
> datagram.\r\n"
> -"  -_ip6- Specifies the IPv6 stack usage mode (Default is IPv4 
> stack).\r\n"
> -"  -_s  - Specifies the source adapter as IPv4 or IPv6 address.\r\n"
> -"  SourceIp - Specifies the IPv4 or IPv6 address of