From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 75838211D35CE for ; Thu, 14 Mar 2019 06:07:42 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BFE3F307E074; Thu, 14 Mar 2019 13:07:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-206.rdu2.redhat.com [10.10.123.206]) by smtp.corp.redhat.com (Postfix) with ESMTP id A2FA64521; Thu, 14 Mar 2019 13:07:39 +0000 (UTC) To: Zhichao Gao , edk2-devel@lists.01.org Cc: Jordan Justen , Ard Biesheuvel , Liming Gao , Sean Brogan , Michael Turner , Bret Barkelew References: <20190314090351.14248-1-zhichao.gao@intel.com> <20190314090351.14248-10-zhichao.gao@intel.com> From: Laszlo Ersek Message-ID: <6fdd54ce-4a99-fd55-35d5-a68606db6c7a@redhat.com> Date: Thu, 14 Mar 2019 14:07:33 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190314090351.14248-10-zhichao.gao@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.42]); Thu, 14 Mar 2019 13:07:41 +0000 (UTC) Subject: Re: [PATCH 09/17] OvmfPkg/PlatformDebugLibIoPort: Add a new api DebugVPrint X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 Mar 2019 13:07:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Cc: Liming Gao > Cc: Sean Brogan > Cc: Michael Turner > Cc: Bret Barkelew > --- > 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.
> + Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> Copyright (c) 2012, Red Hat, Inc.
> 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