[PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[Laszlo Ersek]

Tapan reported
<https://tianocore.acgmultimedia.com/show_bug.cgi?id=112>. Since I had
encountered the problem myself, and now managed to find a tiny time
slot, I looked into it. Patch #2 is the fix; patch #1 cleans up a small
wart that I came across while studying the code.

Public branch: <https://github.com/lersek/edk2/commits/childctrl>.

Tapan, can you please test the series, and respond with your Tested-by?
Thanks!

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Tapan Shah <tapandshah@hpe.com>

Cheers,
Laszlo

Laszlo Ersek (2):
  ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr
    parsing
  ShellPkg/UefiHandleParsingLib: fix retval for empty child controller
    array

 ShellPkg/Include/Library/HandleParsingLib.h                  |  4 ++--
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 15 +++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.9.2

[PATCH 1/2] ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr parsing

[Laszlo Ersek]

"MatchingHandleCount" is an output parameter of
ParseHandleDatabaseForChildControllers().

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Tapan Shah <tapandshah@hpe.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ShellPkg/Include/Library/HandleParsingLib.h                  | 4 ++--
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/ShellPkg/Include/Library/HandleParsingLib.h b/ShellPkg/Include/Library/HandleParsingLib.h
index 441f65fffe83..79dcc9cc5a55 100644
--- a/ShellPkg/Include/Library/HandleParsingLib.h
+++ b/ShellPkg/Include/Library/HandleParsingLib.h
@@ -303,24 +303,24 @@ ParseHandleDatabaseForChildDevices(
   );
 
 /**
   Gets handles for any child controllers of the passed in controller.
 
   @param[in] ControllerHandle       The handle of the "parent controller".
-  @param[in] MatchingHandleCount    The pointer to the number of handles in
+  @param[out] MatchingHandleCount   The pointer to the number of handles in
                                     MatchingHandleBuffer on return.
   @param[out] MatchingHandleBuffer  The buffer containing handles on a successful
                                     return.
   @retval EFI_SUCCESS               The operation was successful.
   @sa ParseHandleDatabaseByRelationship
 **/
 EFI_STATUS
 EFIAPI
 ParseHandleDatabaseForChildControllers(
   IN CONST EFI_HANDLE       ControllerHandle,
-  IN UINTN                  *MatchingHandleCount,
+  OUT UINTN                 *MatchingHandleCount,
   OUT EFI_HANDLE            **MatchingHandleBuffer OPTIONAL
   );
 
 
 /**
   Function to retrieve the human-friendly index of a given handle.  If the handle
diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index 3fb55df8cc14..e11a3ccceab3 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -2720,25 +2720,25 @@ ParseHandleDatabaseByRelationship (
 }
 
 /**
   Gets handles for any child controllers of the passed in controller.
 
   @param[in] ControllerHandle       The handle of the "parent controller"
-  @param[in] MatchingHandleCount    Pointer to the number of handles in
+  @param[out] MatchingHandleCount   Pointer to the number of handles in
                                     MatchingHandleBuffer on return.
   @param[out] MatchingHandleBuffer  Buffer containing handles on a successful
                                     return.
 
 
   @retval EFI_SUCCESS               The operation was sucessful.
 **/
 EFI_STATUS
 EFIAPI
 ParseHandleDatabaseForChildControllers(
   IN CONST EFI_HANDLE       ControllerHandle,
-  IN UINTN                  *MatchingHandleCount,
+  OUT UINTN                 *MatchingHandleCount,
   OUT EFI_HANDLE            **MatchingHandleBuffer OPTIONAL
   )
 {
   EFI_STATUS  Status;
   UINTN       HandleIndex;
   UINTN       DriverBindingHandleCount;
-- 
2.9.2

[PATCH 2/2] ShellPkg/UefiHandleParsingLib: fix retval for empty child controller array

[Laszlo Ersek]

The ParseHandleDatabaseForChildControllers() function intends to work like
this:

(1) It allocates a "HandleBufferForReturn" local array that's guaranteed
    to be big enough for all found handles,

(2) it collects the handles, both counting them in the (mandatory)
    "MatchingHandleCount" output parameter, and saving them in the local
    "HandleBufferForReturn" array,

(3) if the caller is not interested in the actual handles, then
    "HandleBufferForReturn" is released,

(4) if the caller is interested in the handles, and we've found some, then
    "HandleBufferForReturn" is passed out through the
    "MatchingHandleBuffer" output parameter,

(5) if the caller is interested in the actual handles, but we've found
    none, then the "MatchingHandleBuffer" output parameter is set to NULL.

The ASSERT() at the end of the function makes this clear, but the
implementation does not conform to (5). Fix it.

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Tapan Shah <tapandshah@hpe.com>
Reported-by: Tapan Shah <tapandshah@hpe.com>
Ref: https://tianocore.acgmultimedia.com/show_bug.cgi?id=112
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
index e11a3ccceab3..695d090926e1 100644
--- a/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
+++ b/ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c
@@ -2799,17 +2799,24 @@ ParseHandleDatabaseForChildControllers(
 
     FreePool (ChildControllerHandleBuffer);
   }
 
   FreePool (DriverBindingHandleBuffer);
 
+  if (MatchingHandleBuffer == NULL || *MatchingHandleCount == 0) {
+    //
+    // The caller is not interested in the actual handles, or we've found none.
+    //
+    FreePool (HandleBufferForReturn);
+    HandleBufferForReturn = NULL;
+  }
+
   if (MatchingHandleBuffer != NULL) {
     *MatchingHandleBuffer = HandleBufferForReturn;
-  } else {
-    FreePool(HandleBufferForReturn);
   }
+
   ASSERT ((MatchingHandleBuffer == NULL) ||
           (*MatchingHandleCount == 0 && *MatchingHandleBuffer == NULL) ||
           (*MatchingHandleCount != 0 && *MatchingHandleBuffer != NULL));
 
   return (EFI_SUCCESS);
 }
-- 
2.9.2

Re: [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[Carsey, Jaben]

Thanks and for the Series.

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

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, September 08, 2016 9:14 AM
> To: edk2-devel@ml01.01.org
> Cc: Carsey, Jaben <jaben.carsey@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [edk2] [PATCH 0/2] ShellPkg: fix assertion failure encountered with
> "devtree" and "dh -d -v"
> Importance: High
> 
> Tapan reported
> <https://tianocore.acgmultimedia.com/show_bug.cgi?id=112>. Since I had
> encountered the problem myself, and now managed to find a tiny time
> slot, I looked into it. Patch #2 is the fix; patch #1 cleans up a small
> wart that I came across while studying the code.
> 
> Public branch: <https://github.com/lersek/edk2/commits/childctrl>.
> 
> Tapan, can you please test the series, and respond with your Tested-by?
> Thanks!
> 
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Tapan Shah <tapandshah@hpe.com>
> 
> Cheers,
> Laszlo
> 
> Laszlo Ersek (2):
>   ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr
>     parsing
>   ShellPkg/UefiHandleParsingLib: fix retval for empty child controller
>     array
> 
>  ShellPkg/Include/Library/HandleParsingLib.h                  |  4 ++--
>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 15
> +++++++++++----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> --
> 2.9.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[Shah, Tapan]

Missing company's copyright message in both changed filed. Code works fine on NT32.
 
Reviewed-by: Tapan Shah <tapandshah@hpe.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, September 08, 2016 11:14 AM
To: edk2-devel@ml01.01.org
Cc: Jaben Carsey <jaben.carsey@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
Subject: [edk2] [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

Tapan reported
<https://tianocore.acgmultimedia.com/show_bug.cgi?id=112>. Since I had
encountered the problem myself, and now managed to find a tiny time
slot, I looked into it. Patch #2 is the fix; patch #1 cleans up a small
wart that I came across while studying the code.

Public branch: <https://github.com/lersek/edk2/commits/childctrl>.

Tapan, can you please test the series, and respond with your Tested-by?
Thanks!

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Tapan Shah <tapandshah@hpe.com>

Cheers,
Laszlo

Laszlo Ersek (2):
  ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr
    parsing
  ShellPkg/UefiHandleParsingLib: fix retval for empty child controller
    array

 ShellPkg/Include/Library/HandleParsingLib.h                  |  4 ++--
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 15 +++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.9.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[Shah, Tapan]

Typo..... Missing company's copyright message in both changed files*

-----Original Message-----
From: Shah, Tapan 
Sent: Thursday, September 08, 2016 12:09 PM
To: 'Laszlo Ersek' <lersek@redhat.com>; edk2-devel@ml01.01.org
Cc: Jaben Carsey <jaben.carsey@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
Subject: RE: [edk2] [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

Missing company's copyright message in both changed filed. Code works fine on NT32.
 
Reviewed-by: Tapan Shah <tapandshah@hpe.com>

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, September 08, 2016 11:14 AM
To: edk2-devel@ml01.01.org
Cc: Jaben Carsey <jaben.carsey@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
Subject: [edk2] [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

Tapan reported
<https://tianocore.acgmultimedia.com/show_bug.cgi?id=112>. Since I had encountered the problem myself, and now managed to find a tiny time slot, I looked into it. Patch #2 is the fix; patch #1 cleans up a small wart that I came across while studying the code.

Public branch: <https://github.com/lersek/edk2/commits/childctrl>.

Tapan, can you please test the series, and respond with your Tested-by?
Thanks!

Cc: Jaben Carsey <jaben.carsey@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Tapan Shah <tapandshah@hpe.com>

Cheers,
Laszlo

Laszlo Ersek (2):
  ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr
    parsing
  ShellPkg/UefiHandleParsingLib: fix retval for empty child controller
    array

 ShellPkg/Include/Library/HandleParsingLib.h                  |  4 ++--
 ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 15 +++++++++++----
 2 files changed, 13 insertions(+), 6 deletions(-)

--
2.9.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[Laszlo Ersek]

On 09/08/16 19:09, Shah, Tapan wrote:
> Typo..... Missing company's copyright message in both changed files*

Bah, I don't care about that for files that exist, the git log is just fine for that. :) I am aware that a copyright notice is required for new files, and I do comply with that absolutely. I also never touch copyright lines belonging to other companies (big no-no).

But, I generally don't litter my patches with RH copyright lines (or copyright line updates) for files that already exist -- other companies may require their associates to do so (I recall such hunks from Intel), but we don't do that (not just in edk2, but as far as I am aware, in any of the other projects where we modify files -- QEMU and the Linux kernel are the most direct examples I can think of).

For example, in Linux, the "arch/x86/kvm/vmx.c" file still says:

/*
 * Kernel-based Virtual Machine driver for Linux
 *
 * This module enables machines with Intel VT-x extensions to run virtual
 * machines without emulation or binary translation.
 *
 * Copyright (C) 2006 Qumranet, Inc.
 * Copyright 2010 Red Hat, Inc. and/or its affiliates.
 *
 * Authors:
 *   Avi Kivity   <avi@qumranet.com>
 *   Yaniv Kamay  <yaniv@qumranet.com>
 *
 * This work is licensed under the terms of the GNU GPL, version 2.  See
 * the COPYING file in the top-level directory.
 *
 */

Funnily enough, the Red Hat line was added in kernel commit 221d059d15f1c ("KVM: Update Red Hat copyrights") in 2010, even though RH acquired Qumranet in 2008 (according to Wikipedia).

But, I digress. I think the current patches satisfy "ShellPkg/Contributions.txt". I do thank you for pointing this out: if I were responsible for sticking the explicit (C) RH notice in every preexistent file I touch, I'd definitely fix up the patches now.

Thank you!
Laszlo

> -----Original Message-----
> From: Shah, Tapan 
> Sent: Thursday, September 08, 2016 12:09 PM
> To: 'Laszlo Ersek' <lersek@redhat.com>; edk2-devel@ml01.01.org
> Cc: Jaben Carsey <jaben.carsey@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
> Subject: RE: [edk2] [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"
> 
> Missing company's copyright message in both changed filed. Code works fine on NT32.
>  
> Reviewed-by: Tapan Shah <tapandshah@hpe.com>
> 
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, September 08, 2016 11:14 AM
> To: edk2-devel@ml01.01.org
> Cc: Jaben Carsey <jaben.carsey@intel.com>; Ruiyu Ni <ruiyu.ni@intel.com>
> Subject: [edk2] [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"
> 
> Tapan reported
> <https://tianocore.acgmultimedia.com/show_bug.cgi?id=112>. Since I had encountered the problem myself, and now managed to find a tiny time slot, I looked into it. Patch #2 is the fix; patch #1 cleans up a small wart that I came across while studying the code.
> 
> Public branch: <https://github.com/lersek/edk2/commits/childctrl>.
> 
> Tapan, can you please test the series, and respond with your Tested-by?
> Thanks!
> 
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Tapan Shah <tapandshah@hpe.com>
> 
> Cheers,
> Laszlo
> 
> Laszlo Ersek (2):
>   ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr
>     parsing
>   ShellPkg/UefiHandleParsingLib: fix retval for empty child controller
>     array
> 
>  ShellPkg/Include/Library/HandleParsingLib.h                  |  4 ++--
>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 15 +++++++++++----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 
> --
> 2.9.2
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 0/2] ShellPkg: fix assertion failure encountered with "devtree" and "dh -d -v"

[Laszlo Ersek]

On 09/08/16 18:14, Laszlo Ersek wrote:
> Tapan reported
> <https://tianocore.acgmultimedia.com/show_bug.cgi?id=112>. Since I had
> encountered the problem myself, and now managed to find a tiny time
> slot, I looked into it. Patch #2 is the fix; patch #1 cleans up a small
> wart that I came across while studying the code.
> 
> Public branch: <https://github.com/lersek/edk2/commits/childctrl>.
> 
> Tapan, can you please test the series, and respond with your Tested-by?
> Thanks!
> 
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Tapan Shah <tapandshah@hpe.com>
> 
> Cheers,
> Laszlo
> 
> Laszlo Ersek (2):
>   ShellPkg/UefiHandleParsingLib: fix IN/OUT notation in child ctrlr
>     parsing
>   ShellPkg/UefiHandleParsingLib: fix retval for empty child controller
>     array
> 
>  ShellPkg/Include/Library/HandleParsingLib.h                  |  4 ++--
>  ShellPkg/Library/UefiHandleParsingLib/UefiHandleParsingLib.c | 15 +++++++++++----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 

Thanks everyone for the reviews & testing, series pushed as
1b3be4a12e7d..7eb3bb6c552d.

Cheers!
Laszlo