Re: [PATCH 09/17] OvmfPkg/PlatformDebugLibIoPort: Add a new api DebugVPrint

[Laszlo Ersek <lersek@redhat.com>]

On 03/14/19 10:03, Zhichao Gao wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1395
> 
> Add a new api DebugVPrint implementation in the
> DebugLib instance. This api would expose a print
> routine with VaList parameter.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Sean Brogan <sean.brogan@microsoft.com>
> Cc: Michael Turner <Michael.Turner@microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> ---
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c | 41 +++++++++++++++++++----
>  1 file changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> index 36cde54976..1e64eeec11 100644
> --- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> +++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
> @@ -2,7 +2,7 @@
>    Base Debug library instance for QEMU debug port.
>    It uses PrintLib to send debug messages to a fixed I/O port.
>  
> -  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2012, Red Hat, Inc.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD License
> @@ -51,9 +51,41 @@ DebugPrint (
>    IN  CONST CHAR8  *Format,
>    ...
>    )
> +{
> +  VA_LIST         Marker;
> +
> +  ASSERT(Format != NULL);

(1) I think this ASSERT() is redundant.  The rest of this function will
work fine with the ASSERT() removed, and the DebugVPrint() function will
itself contain an ASSERT(), with the same condition. So I'd prefer
removing this one. (The function-level comments will remain valid.)

> +
> +  VA_START(Marker, Format);
> +  DebugVPrint(ErrorLevel, Format, Marker);
> +  VA_END(Marker);

(2) Please insert a space character before each opening parenthesis.

> +}
> +
> +/**
> +  Prints a debug message to the debug output device if the specified error level is enabled.
> +
> +  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function
> +  GetDebugPrintErrorLevel (), then print the message specified by Format and the
> +  associated variable argument list to the debug output device.
> +
> +  If Format is NULL, then ASSERT().
> +  If the length of the message string specificed by Format is larger than the maximum allowable
> +  record length, then directly return and not print it.

(3) The last sentence is not enforced by the implementation.

We should
- either implement the check,
- or extend the sentence, saying that there is no such limit in this
library instance,
- or remove the sentence.

I think I'd prefer simply removing the sentence, given that the
DebugPrint() comments lack it as well.

> +
> +  @param  ErrorLevel    The error level of the debug message.
> +  @param  Format        Format string for the debug message to print.
> +  @param  VaListMarker  VA_LIST marker for the variable argument list.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugVPrint (
> +  IN  UINTN        ErrorLevel,
> +  IN  CONST CHAR8  *Format,
> +  IN  VA_LIST       VaListMarker
> +  )
>  {
>    CHAR8    Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> -  VA_LIST  Marker;
>    UINTN    Length;
>  
>    //
> @@ -72,9 +104,7 @@ DebugPrint (
>    //
>    // Convert the DEBUG() message to an ASCII String
>    //
> -  VA_START (Marker, Format);
> -  Length = AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
> -  VA_END (Marker);
> +  Length = AsciiVSPrint (Buffer, sizeof (Buffer), Format, VaListMarker);
>  
>    //
>    // Send the print string to the debug I/O port
> @@ -82,7 +112,6 @@ DebugPrint (
>    IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
>  }
>  
> -
>  /**
>    Prints an assert message containing a filename, line number, and description.
>    This may be followed by a breakpoint or a dead loop.
> 

(4) This hunk (the last hunk of the patch), which only removes an empty
line, looks superfluous to me. I suggest dropping it.

Thanks
Laszlo