* [PATCH] Ifconfig : Fixed False information about Media State. @ 2017-10-05 6:36 Meenakshi Aggarwal 2017-10-05 15:01 ` Carsey, Jaben 2017-10-06 4:40 ` [PATCH v2] " Meenakshi Aggarwal 0 siblings, 2 replies; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-05 6:36 UTC (permalink / raw) To: edk2-devel, jiaxin.wu, jaben.carsey, ruiyu.ni Issue : We were setting MediaPresent as TRUE (default), so even if NetLibDetectMedia() did not set MediaPresent Flag as TRUE, ifconfig always display Media State as 'Media Present' Fix : Set MediaPresent as FALSE before calling NetLibDetectMedia() Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> --- ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c index 4db07b2..7c05b68 100644 --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c @@ -554,8 +554,6 @@ IfConfigShowInterfaceInfo ( EFI_IPv4_ADDRESS Gateway; UINT32 Index; - MediaPresent = TRUE; - if (IsListEmpty (IfList)) { ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INVALID_INTERFACE), gShellNetwork1HiiHandle); } @@ -576,6 +574,8 @@ IfConfigShowInterfaceInfo ( // // Get Media State. // + MediaPresent = FALSE; + NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); if (!MediaPresent) { ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] Ifconfig : Fixed False information about Media State. 2017-10-05 6:36 [PATCH] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal @ 2017-10-05 15:01 ` Carsey, Jaben 2017-10-05 17:10 ` Meenakshi Aggarwal 2017-10-06 4:40 ` [PATCH v2] " Meenakshi Aggarwal 1 sibling, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2017-10-05 15:01 UTC (permalink / raw) To: Meenakshi Aggarwal, edk2-devel@lists.01.org, Wu, Jiaxin, Ni, Ruiyu Is there a reason to move the assignment in the function? I think our coding guidelines specify initialize variables up top. -Jaben > -----Original Message----- > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > Sent: Wednesday, October 04, 2017 11:37 PM > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Subject: [PATCH] Ifconfig : Fixed False information about Media State. > Importance: High > > Issue : We were setting MediaPresent as TRUE (default), so > even if NetLibDetectMedia() did not set MediaPresent Flag as TRUE, > ifconfig always display Media State as 'Media Present' > > Fix : Set MediaPresent as FALSE before calling NetLibDetectMedia() > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > index 4db07b2..7c05b68 100644 > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > @@ -554,8 +554,6 @@ IfConfigShowInterfaceInfo ( > EFI_IPv4_ADDRESS Gateway; > UINT32 Index; > > - MediaPresent = TRUE; > - > if (IsListEmpty (IfList)) { > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INVALID_INTERFACE), gShellNetwork1HiiHandle); > } > @@ -576,6 +574,8 @@ IfConfigShowInterfaceInfo ( > // > // Get Media State. > // > + MediaPresent = FALSE; > + > NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > if (!MediaPresent) { > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > disconnected"); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Ifconfig : Fixed False information about Media State. 2017-10-05 15:01 ` Carsey, Jaben @ 2017-10-05 17:10 ` Meenakshi Aggarwal 2017-10-05 20:26 ` Carsey, Jaben 0 siblings, 1 reply; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-05 17:10 UTC (permalink / raw) To: Carsey, Jaben, edk2-devel@lists.01.org, Wu, Jiaxin, Ni, Ruiyu Yes, its moved in the function because NetLibDetectMedia() function is called in a loop (number of active interfaces). So we need to initialize it to FALSE every time before calling NetLibDetectMedia() else if it was set TRUE for some interface then it will remain so for remaining. Thanks, Meenakshi > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > Sent: Thursday, October 05, 2017 8:32 PM > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > <ruiyu.ni@intel.com> > Subject: RE: [PATCH] Ifconfig : Fixed False information about Media State. > > Is there a reason to move the assignment in the function? I think our coding > guidelines specify initialize variables up top. > > -Jaben > > > -----Original Message----- > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > Sent: Wednesday, October 04, 2017 11:37 PM > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > Subject: [PATCH] Ifconfig : Fixed False information about Media State. > > Importance: High > > > > Issue : We were setting MediaPresent as TRUE (default), so even if > > NetLibDetectMedia() did not set MediaPresent Flag as TRUE, ifconfig > > always display Media State as 'Media Present' > > > > Fix : Set MediaPresent as FALSE before calling NetLibDetectMedia() > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > --- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > index 4db07b2..7c05b68 100644 > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > @@ -554,8 +554,6 @@ IfConfigShowInterfaceInfo ( > > EFI_IPv4_ADDRESS Gateway; > > UINT32 Index; > > > > - MediaPresent = TRUE; > > - > > if (IsListEmpty (IfList)) { > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INVALID_INTERFACE), gShellNetwork1HiiHandle); > > } > > @@ -576,6 +574,8 @@ IfConfigShowInterfaceInfo ( > > // > > // Get Media State. > > // > > + MediaPresent = FALSE; > > + > > NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > if (!MediaPresent) { > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > > disconnected"); > > -- > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Ifconfig : Fixed False information about Media State. 2017-10-05 17:10 ` Meenakshi Aggarwal @ 2017-10-05 20:26 ` Carsey, Jaben 2017-10-06 4:21 ` Meenakshi Aggarwal 0 siblings, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2017-10-05 20:26 UTC (permalink / raw) To: Meenakshi Aggarwal, edk2-devel@lists.01.org, Wu, Jiaxin, Ni, Ruiyu What are the conditions that the function does not successfully set the MediaPresent setting? Should we also be checking the return value of NetLibDetectMedia? > -----Original Message----- > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > Sent: Thursday, October 05, 2017 10:10 AM > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; Wu, > Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: RE: [PATCH] Ifconfig : Fixed False information about Media State. > Importance: High > > Yes, its moved in the function because NetLibDetectMedia() function is > called in a loop (number of active interfaces). > So we need to initialize it to FALSE every time before calling > NetLibDetectMedia() else if it was set TRUE for some interface then it will > remain so for remaining. > > > Thanks, > Meenakshi > > > -----Original Message----- > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > Sent: Thursday, October 05, 2017 8:32 PM > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > > <ruiyu.ni@intel.com> > > Subject: RE: [PATCH] Ifconfig : Fixed False information about Media State. > > > > Is there a reason to move the assignment in the function? I think our > coding > > guidelines specify initialize variables up top. > > > > -Jaben > > > > > -----Original Message----- > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > Sent: Wednesday, October 04, 2017 11:37 PM > > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > > > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > Subject: [PATCH] Ifconfig : Fixed False information about Media State. > > > Importance: High > > > > > > Issue : We were setting MediaPresent as TRUE (default), so even if > > > NetLibDetectMedia() did not set MediaPresent Flag as TRUE, ifconfig > > > always display Media State as 'Media Present' > > > > > > Fix : Set MediaPresent as FALSE before calling NetLibDetectMedia() > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > --- > > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > index 4db07b2..7c05b68 100644 > > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > @@ -554,8 +554,6 @@ IfConfigShowInterfaceInfo ( > > > EFI_IPv4_ADDRESS Gateway; > > > UINT32 Index; > > > > > > - MediaPresent = TRUE; > > > - > > > if (IsListEmpty (IfList)) { > > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INVALID_INTERFACE), gShellNetwork1HiiHandle); > > > } > > > @@ -576,6 +574,8 @@ IfConfigShowInterfaceInfo ( > > > // > > > // Get Media State. > > > // > > > + MediaPresent = FALSE; > > > + > > > NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > > if (!MediaPresent) { > > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > L"Media > > > disconnected"); > > > -- > > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] Ifconfig : Fixed False information about Media State. 2017-10-05 20:26 ` Carsey, Jaben @ 2017-10-06 4:21 ` Meenakshi Aggarwal 0 siblings, 0 replies; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-06 4:21 UTC (permalink / raw) To: Carsey, Jaben, edk2-devel@lists.01.org, Wu, Jiaxin, Ni, Ruiyu Yes, we can check return value of NetLibDetectMedia(), then we don't need to initialize it with FALSE in loop. NetLibDetectMedia() sets the MediaPresent Flag to TRUE in case of success, and don't do anything with it in failure cases. I will send the patch, let me know if you agreed with this approach. Thanks, Meenakshi > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > Sent: Friday, October 06, 2017 1:57 AM > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > <ruiyu.ni@intel.com> > Subject: RE: [PATCH] Ifconfig : Fixed False information about Media State. > > What are the conditions that the function does not successfully set the > MediaPresent setting? Should we also be checking the return value of > NetLibDetectMedia? > > > -----Original Message----- > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > Sent: Thursday, October 05, 2017 10:10 AM > > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; > > Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > Subject: RE: [PATCH] Ifconfig : Fixed False information about Media State. > > Importance: High > > > > Yes, its moved in the function because NetLibDetectMedia() function is > > called in a loop (number of active interfaces). > > So we need to initialize it to FALSE every time before calling > > NetLibDetectMedia() else if it was set TRUE for some interface then it > > will remain so for remaining. > > > > > > Thanks, > > Meenakshi > > > > > -----Original Message----- > > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > > Sent: Thursday, October 05, 2017 8:32 PM > > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > > > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > > > <ruiyu.ni@intel.com> > > > Subject: RE: [PATCH] Ifconfig : Fixed False information about Media State. > > > > > > Is there a reason to move the assignment in the function? I think > > > our > > coding > > > guidelines specify initialize variables up top. > > > > > > -Jaben > > > > > > > -----Original Message----- > > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > > Sent: Wednesday, October 04, 2017 11:37 PM > > > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; > > > > Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu > > > > <ruiyu.ni@intel.com> > > > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > > Subject: [PATCH] Ifconfig : Fixed False information about Media State. > > > > Importance: High > > > > > > > > Issue : We were setting MediaPresent as TRUE (default), so even if > > > > NetLibDetectMedia() did not set MediaPresent Flag as TRUE, > > > > ifconfig always display Media State as 'Media Present' > > > > > > > > Fix : Set MediaPresent as FALSE before calling NetLibDetectMedia() > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > > --- > > > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git > > > > a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > index 4db07b2..7c05b68 100644 > > > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > @@ -554,8 +554,6 @@ IfConfigShowInterfaceInfo ( > > > > EFI_IPv4_ADDRESS Gateway; > > > > UINT32 Index; > > > > > > > > - MediaPresent = TRUE; > > > > - > > > > if (IsListEmpty (IfList)) { > > > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INVALID_INTERFACE), gShellNetwork1HiiHandle); > > > > } > > > > @@ -576,6 +574,8 @@ IfConfigShowInterfaceInfo ( > > > > // > > > > // Get Media State. > > > > // > > > > + MediaPresent = FALSE; > > > > + > > > > NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > > > if (!MediaPresent) { > > > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > L"Media > > > > disconnected"); > > > > -- > > > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] Ifconfig : Fixed False information about Media State. 2017-10-05 6:36 [PATCH] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal 2017-10-05 15:01 ` Carsey, Jaben @ 2017-10-06 4:40 ` Meenakshi Aggarwal 2017-10-06 4:48 ` [PATCH v3] *** Ifconfig : Fixed False information about Media State *** Meenakshi Aggarwal 1 sibling, 1 reply; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-06 4:40 UTC (permalink / raw) To: edk2-devel, jiaxin.wu, jaben.carsey, ruiyu.ni Issue : We were setting MediaPresent as TRUE (default), so even if NetLibDetectMedia() did not set MediaPresent Flag as TRUE, ifconfig always display Media State as 'Media Present' Fix : Set MediaPresent as FALSE before calling NetLibDetectMedia() Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> --- ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c index 4db07b2..90ca724 100644 --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( // // Get Media State. // - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); - if (!MediaPresent) { - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, &MediaPresent)) { + if (!MediaPresent) { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); + } else { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media present"); + } } else { - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media present"); + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); } // -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3] *** Ifconfig : Fixed False information about Media State *** 2017-10-06 4:40 ` [PATCH v2] " Meenakshi Aggarwal @ 2017-10-06 4:48 ` Meenakshi Aggarwal 2017-10-06 4:48 ` [PATCH v3] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal 0 siblings, 1 reply; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-06 4:48 UTC (permalink / raw) To: edk2-devel, jiaxin.wu, jaben.carsey, ruiyu.ni Corrected commit message Meenakshi Aggarwal (1): Ifconfig : Fixed False information about Media State. ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3] Ifconfig : Fixed False information about Media State. 2017-10-06 4:48 ` [PATCH v3] *** Ifconfig : Fixed False information about Media State *** Meenakshi Aggarwal @ 2017-10-06 4:48 ` Meenakshi Aggarwal 2017-10-06 14:01 ` Carsey, Jaben 2017-10-10 14:07 ` [PATCH v4] " Meenakshi Aggarwal 0 siblings, 2 replies; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-06 4:48 UTC (permalink / raw) To: edk2-devel, jiaxin.wu, jaben.carsey, ruiyu.ni Issue : We were setting MediaPresent as TRUE (default) and not checking return status of NetLibDetectMedia(). NetLibDetectMedia() sets MediaPresent FLAG in case of success only and dont change flag on error. So, Media State will display as 'Media Present', in case of error also. Fix : Check return value of NetLibDetectMedia() Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> --- ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c index 4db07b2..90ca724 100644 --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( // // Get Media State. // - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); - if (!MediaPresent) { - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, &MediaPresent)) { + if (!MediaPresent) { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); + } else { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media present"); + } } else { - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media present"); + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); } // -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3] Ifconfig : Fixed False information about Media State. 2017-10-06 4:48 ` [PATCH v3] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal @ 2017-10-06 14:01 ` Carsey, Jaben 2017-10-08 8:49 ` Meenakshi Aggarwal 2017-10-10 14:07 ` [PATCH v4] " Meenakshi Aggarwal 1 sibling, 1 reply; 15+ messages in thread From: Carsey, Jaben @ 2017-10-06 14:01 UTC (permalink / raw) To: Meenakshi Aggarwal, edk2-devel@lists.01.org, Wu, Jiaxin, Ni, Ruiyu Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> Do you know under what conditions the API will fail? Is it worth saying something like media stats unknown when the function fails? Ray, What do you think? > -----Original Message----- > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > Sent: Thursday, October 05, 2017 9:48 PM > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > Subject: [PATCH v3] Ifconfig : Fixed False information about Media State. > Importance: High > > Issue : We were setting MediaPresent as TRUE (default) and > not checking return status of NetLibDetectMedia(). > NetLibDetectMedia() sets MediaPresent FLAG in case of success > only and dont change flag on error. > So, Media State will display as 'Media Present', in case of > error also. > > Fix : Check return value of NetLibDetectMedia() > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 +++++++-- > -- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > index 4db07b2..90ca724 100644 > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( > // > // Get Media State. > // > - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > - if (!MediaPresent) { > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > disconnected"); > + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, > &MediaPresent)) { > + if (!MediaPresent) { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > disconnected"); > + } else { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > present"); > + } > } else { > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > present"); > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > disconnected"); > } > > // > -- > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] Ifconfig : Fixed False information about Media State. 2017-10-06 14:01 ` Carsey, Jaben @ 2017-10-08 8:49 ` Meenakshi Aggarwal 2017-10-09 1:29 ` Wu, Jiaxin 0 siblings, 1 reply; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-08 8:49 UTC (permalink / raw) To: Carsey, Jaben, edk2-devel@lists.01.org, Wu, Jiaxin, Ni, Ruiyu It is hard to say when can an API fail because its dependent on implementation. > -----Original Message----- > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > Sent: Friday, October 06, 2017 7:32 PM > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > <ruiyu.ni@intel.com> > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media State. > > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> Do you know under > what conditions the API will fail? Is it worth saying something like media stats > unknown when the function fails? > > Ray, > > What do you think? > > > > -----Original Message----- > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > Sent: Thursday, October 05, 2017 9:48 PM > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > Subject: [PATCH v3] Ifconfig : Fixed False information about Media State. > > Importance: High > > > > Issue : We were setting MediaPresent as TRUE (default) and not > > checking return status of NetLibDetectMedia(). > > NetLibDetectMedia() sets MediaPresent FLAG in case of success only and > > dont change flag on error. > > So, Media State will display as 'Media Present', in case of error > > also. > > > > Fix : Check return value of NetLibDetectMedia() > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > --- > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 > > +++++++-- > > -- > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > index 4db07b2..90ca724 100644 > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( > > // > > // Get Media State. > > // > > - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > - if (!MediaPresent) { > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > > disconnected"); > > + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, > > &MediaPresent)) { > > + if (!MediaPresent) { > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > > disconnected"); > > + } else { > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > > present"); > > + } > > } else { > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > > present"); > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > > disconnected"); > > } > > > > // > > -- > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] Ifconfig : Fixed False information about Media State. 2017-10-08 8:49 ` Meenakshi Aggarwal @ 2017-10-09 1:29 ` Wu, Jiaxin 2017-10-09 2:09 ` Ni, Ruiyu 0 siblings, 1 reply; 15+ messages in thread From: Wu, Jiaxin @ 2017-10-09 1:29 UTC (permalink / raw) To: Meenakshi Aggarwal, Carsey, Jaben, edk2-devel@lists.01.org, Ni, Ruiyu I agree with Jaben. If NetLibDetectMedia return error status, we can output as below: ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media state unknown"); Thanks, Jiaxin > -----Original Message----- > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > Sent: Sunday, October 8, 2017 4:49 PM > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; Wu, > Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media State. > > It is hard to say when can an API fail because its dependent on > implementation. > > > > -----Original Message----- > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > Sent: Friday, October 06, 2017 7:32 PM > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > > <ruiyu.ni@intel.com> > > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media > State. > > > > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> Do you know under > > what conditions the API will fail? Is it worth saying something like media > stats > > unknown when the function fails? > > > > Ray, > > > > What do you think? > > > > > > > -----Original Message----- > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > Sent: Thursday, October 05, 2017 9:48 PM > > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > > > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > Subject: [PATCH v3] Ifconfig : Fixed False information about Media State. > > > Importance: High > > > > > > Issue : We were setting MediaPresent as TRUE (default) and not > > > checking return status of NetLibDetectMedia(). > > > NetLibDetectMedia() sets MediaPresent FLAG in case of success only and > > > dont change flag on error. > > > So, Media State will display as 'Media Present', in case of error > > > also. > > > > > > Fix : Check return value of NetLibDetectMedia() > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > --- > > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 > > > +++++++-- > > > -- > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > index 4db07b2..90ca724 100644 > > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( > > > // > > > // Get Media State. > > > // > > > - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > > - if (!MediaPresent) { > > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > L"Media > > > disconnected"); > > > + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, > > > &MediaPresent)) { > > > + if (!MediaPresent) { > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > L"Media > > > disconnected"); > > > + } else { > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > L"Media > > > present"); > > > + } > > > } else { > > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > L"Media > > > present"); > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > L"Media > > > disconnected"); > > > } > > > > > > // > > > -- > > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] Ifconfig : Fixed False information about Media State. 2017-10-09 1:29 ` Wu, Jiaxin @ 2017-10-09 2:09 ` Ni, Ruiyu 2017-10-09 3:21 ` Wu, Jiaxin 0 siblings, 1 reply; 15+ messages in thread From: Ni, Ruiyu @ 2017-10-09 2:09 UTC (permalink / raw) To: Wu, Jiaxin, Meenakshi Aggarwal, Carsey, Jaben, edk2-devel@lists.01.org Do you need to put all the hard-code string in UNI file for localization? Thanks/Ray > -----Original Message----- > From: Wu, Jiaxin > Sent: Monday, October 9, 2017 9:29 AM > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Carsey, Jaben > <jaben.carsey@intel.com>; edk2-devel@lists.01.org; Ni, Ruiyu > <ruiyu.ni@intel.com> > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media State. > > I agree with Jaben. If NetLibDetectMedia return error status, we can output as > below: > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media state > unknown"); > > Thanks, > Jiaxin > > > -----Original Message----- > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > Sent: Sunday, October 8, 2017 4:49 PM > > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; > > Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media State. > > > > It is hard to say when can an API fail because its dependent on > > implementation. > > > > > > > -----Original Message----- > > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > > Sent: Friday, October 06, 2017 7:32 PM > > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > > > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > > > <ruiyu.ni@intel.com> > > > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about > > > Media > > State. > > > > > > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> Do you know under > > > what conditions the API will fail? Is it worth saying something like > > > media > > stats > > > unknown when the function fails? > > > > > > Ray, > > > > > > What do you think? > > > > > > > > > > -----Original Message----- > > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > > Sent: Thursday, October 05, 2017 9:48 PM > > > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; > > > > Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu > > > > <ruiyu.ni@intel.com> > > > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > > Subject: [PATCH v3] Ifconfig : Fixed False information about Media State. > > > > Importance: High > > > > > > > > Issue : We were setting MediaPresent as TRUE (default) and not > > > > checking return status of NetLibDetectMedia(). > > > > NetLibDetectMedia() sets MediaPresent FLAG in case of success only > > > > and dont change flag on error. > > > > So, Media State will display as 'Media Present', in case of error > > > > also. > > > > > > > > Fix : Check return value of NetLibDetectMedia() > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > > --- > > > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 > > > > +++++++-- > > > > -- > > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > > > diff --git > > > > a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > index 4db07b2..90ca724 100644 > > > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( > > > > // > > > > // Get Media State. > > > > // > > > > - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > > > - if (!MediaPresent) { > > > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > L"Media > > > > disconnected"); > > > > + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, > > > > &MediaPresent)) { > > > > + if (!MediaPresent) { > > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > L"Media > > > > disconnected"); > > > > + } else { > > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > L"Media > > > > present"); > > > > + } > > > > } else { > > > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > L"Media > > > > present"); > > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > L"Media > > > > disconnected"); > > > > } > > > > > > > > // > > > > -- > > > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3] Ifconfig : Fixed False information about Media State. 2017-10-09 2:09 ` Ni, Ruiyu @ 2017-10-09 3:21 ` Wu, Jiaxin 0 siblings, 0 replies; 15+ messages in thread From: Wu, Jiaxin @ 2017-10-09 3:21 UTC (permalink / raw) To: Ni, Ruiyu, Meenakshi Aggarwal, Carsey, Jaben, edk2-devel@lists.01.org For me, the hard-code string looks good to me here. I'm also fine if you stick to move in UNI file. Thanks, Jiaxin > -----Original Message----- > From: Ni, Ruiyu > Sent: Monday, October 9, 2017 10:09 AM > To: Wu, Jiaxin <jiaxin.wu@intel.com>; Meenakshi Aggarwal > <meenakshi.aggarwal@nxp.com>; Carsey, Jaben <jaben.carsey@intel.com>; > edk2-devel@lists.01.org > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media State. > > Do you need to put all the hard-code string in UNI file for localization? > > Thanks/Ray > > > -----Original Message----- > > From: Wu, Jiaxin > > Sent: Monday, October 9, 2017 9:29 AM > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Carsey, Jaben > > <jaben.carsey@intel.com>; edk2-devel@lists.01.org; Ni, Ruiyu > > <ruiyu.ni@intel.com> > > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media > State. > > > > I agree with Jaben. If NetLibDetectMedia return error status, we can > output as > > below: > > > > ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > state > > unknown"); > > > > Thanks, > > Jiaxin > > > > > -----Original Message----- > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > Sent: Sunday, October 8, 2017 4:49 PM > > > To: Carsey, Jaben <jaben.carsey@intel.com>; edk2-devel@lists.01.org; > > > Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > > > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about Media > State. > > > > > > It is hard to say when can an API fail because its dependent on > > > implementation. > > > > > > > > > > -----Original Message----- > > > > From: Carsey, Jaben [mailto:jaben.carsey@intel.com] > > > > Sent: Friday, October 06, 2017 7:32 PM > > > > To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; edk2- > > > > devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Ni, Ruiyu > > > > <ruiyu.ni@intel.com> > > > > Subject: RE: [PATCH v3] Ifconfig : Fixed False information about > > > > Media > > > State. > > > > > > > > Reviewed-by: Jaben Carsey <jaben.carsey@intel.com> Do you know > under > > > > what conditions the API will fail? Is it worth saying something like > > > > media > > > stats > > > > unknown when the function fails? > > > > > > > > Ray, > > > > > > > > What do you think? > > > > > > > > > > > > > -----Original Message----- > > > > > From: Meenakshi Aggarwal [mailto:meenakshi.aggarwal@nxp.com] > > > > > Sent: Thursday, October 05, 2017 9:48 PM > > > > > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; > > > > > Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu > > > > > <ruiyu.ni@intel.com> > > > > > Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > > > Subject: [PATCH v3] Ifconfig : Fixed False information about Media > State. > > > > > Importance: High > > > > > > > > > > Issue : We were setting MediaPresent as TRUE (default) and not > > > > > checking return status of NetLibDetectMedia(). > > > > > NetLibDetectMedia() sets MediaPresent FLAG in case of success only > > > > > and dont change flag on error. > > > > > So, Media State will display as 'Media Present', in case of error > > > > > also. > > > > > > > > > > Fix : Check return value of NetLibDetectMedia() > > > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > > > > > > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > > > > > --- > > > > > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 > > > > > +++++++-- > > > > > -- > > > > > 1 file changed, 7 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git > > > > > a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > > index 4db07b2..90ca724 100644 > > > > > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > > > > > @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( > > > > > // > > > > > // Get Media State. > > > > > // > > > > > - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > > > > > - if (!MediaPresent) { > > > > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > > L"Media > > > > > disconnected"); > > > > > + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, > > > > > &MediaPresent)) { > > > > > + if (!MediaPresent) { > > > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > > L"Media > > > > > disconnected"); > > > > > + } else { > > > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > > L"Media > > > > > present"); > > > > > + } > > > > > } else { > > > > > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > > L"Media > > > > > present"); > > > > > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > > > > > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, > > > L"Media > > > > > disconnected"); > > > > > } > > > > > > > > > > // > > > > > -- > > > > > 2.7.4 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v4] Ifconfig : Fixed False information about Media State. 2017-10-06 4:48 ` [PATCH v3] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal 2017-10-06 14:01 ` Carsey, Jaben @ 2017-10-10 14:07 ` Meenakshi Aggarwal 2017-10-11 0:47 ` Wu, Jiaxin 1 sibling, 1 reply; 15+ messages in thread From: Meenakshi Aggarwal @ 2017-10-10 14:07 UTC (permalink / raw) To: edk2-devel, jiaxin.wu, jaben.carsey, ruiyu.ni Issue : We were setting MediaPresent as TRUE (default) and not checking return status of NetLibDetectMedia(). NetLibDetectMedia() sets MediaPresent FLAG in case of success only and dont change flag on error. So, Media State will display as 'Media Present', in case of error also. Fix : Check return value of NetLibDetectMedia(), if error then print "Media State Unknown" Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> --- ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c index 4db07b2..082ab72 100644 --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( // // Get Media State. // - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); - if (!MediaPresent) { - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, &MediaPresent)) { + if (!MediaPresent) { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media disconnected"); + } else { + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media present"); + } } else { - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media present"); + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media state unknown"); } // -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v4] Ifconfig : Fixed False information about Media State. 2017-10-10 14:07 ` [PATCH v4] " Meenakshi Aggarwal @ 2017-10-11 0:47 ` Wu, Jiaxin 0 siblings, 0 replies; 15+ messages in thread From: Wu, Jiaxin @ 2017-10-11 0:47 UTC (permalink / raw) To: Meenakshi Aggarwal, edk2-devel@lists.01.org, Carsey, Jaben, Ni, Ruiyu Reviewed-by: Wu Jiaxin <jiaxin.wu@intel.com> > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Meenakshi Aggarwal > Sent: Tuesday, October 10, 2017 10:08 PM > To: edk2-devel@lists.01.org; Wu, Jiaxin <jiaxin.wu@intel.com>; Carsey, > Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> > Subject: [edk2] [PATCH v4] Ifconfig : Fixed False information about Media > State. > > Issue : We were setting MediaPresent as TRUE (default) and > not checking return status of NetLibDetectMedia(). > NetLibDetectMedia() sets MediaPresent FLAG in case of success > only and dont change flag on error. > So, Media State will display as 'Media Present', in case of > error also. > > Fix : Check return value of NetLibDetectMedia(), if error then > print "Media State Unknown" > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com> > --- > ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c | 11 +++++++-- > -- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > index 4db07b2..082ab72 100644 > --- a/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > +++ b/ShellPkg/Library/UefiShellNetwork1CommandsLib/Ifconfig.c > @@ -576,11 +576,14 @@ IfConfigShowInterfaceInfo ( > // > // Get Media State. > // > - NetLibDetectMedia (IfCb->NicHandle, &MediaPresent); > - if (!MediaPresent) { > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > disconnected"); > + if (EFI_SUCCESS == NetLibDetectMedia (IfCb->NicHandle, > &MediaPresent)) { > + if (!MediaPresent) { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > disconnected"); > + } else { > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > present"); > + } > } else { > - ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > present"); > + ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN > (STR_IFCONFIG_INFO_MEDIA_STATE), gShellNetwork1HiiHandle, L"Media > state unknown"); > } > > // > -- > 2.7.4 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-10-11 0:44 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-05 6:36 [PATCH] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal 2017-10-05 15:01 ` Carsey, Jaben 2017-10-05 17:10 ` Meenakshi Aggarwal 2017-10-05 20:26 ` Carsey, Jaben 2017-10-06 4:21 ` Meenakshi Aggarwal 2017-10-06 4:40 ` [PATCH v2] " Meenakshi Aggarwal 2017-10-06 4:48 ` [PATCH v3] *** Ifconfig : Fixed False information about Media State *** Meenakshi Aggarwal 2017-10-06 4:48 ` [PATCH v3] Ifconfig : Fixed False information about Media State Meenakshi Aggarwal 2017-10-06 14:01 ` Carsey, Jaben 2017-10-08 8:49 ` Meenakshi Aggarwal 2017-10-09 1:29 ` Wu, Jiaxin 2017-10-09 2:09 ` Ni, Ruiyu 2017-10-09 3:21 ` Wu, Jiaxin 2017-10-10 14:07 ` [PATCH v4] " Meenakshi Aggarwal 2017-10-11 0:47 ` Wu, Jiaxin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox