From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::342; helo=mail-wm1-x342.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8E90C21180F2C for ; Tue, 30 Oct 2018 03:43:19 -0700 (PDT) Received: by mail-wm1-x342.google.com with SMTP id h2-v6so7398695wmb.0 for ; Tue, 30 Oct 2018 03:43:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=7Ku8t4HNrzT8lAx25J1bAqC6xpn0JPIvI6yhcUfIX1o=; b=iEl17pmSG8WHWPC6g4+3E0EXCkFeq0SBZBPUVefVWjxRZ0EazsmA8tYfQoN14WA99k 9YYmHMvTAdMvazG7EHTBrR1xwPJBkacjyzdjbiRyk4yyXJbEwDda4C0b/m03h/inEXai lKJWo6XZO2EF5gJyCEEzoYPGUjJ1EeUYtBucc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=7Ku8t4HNrzT8lAx25J1bAqC6xpn0JPIvI6yhcUfIX1o=; b=EH0+ko711rXCZO7Br1XmORkWMRmFKfbGsKi2X8QDJmGZcw9EPzYRpqgnuG/v1QQrJx xmoX38HpGuVNQYFPjTvjiX6llZsj6Fscxd5dxqsz6Stjsm12t1FroMQnoFWv38bpcZAX a+4KT7ExRF8j4M70+ef/iytJ0LZuqoEb5c5w7lwOG0RXOpJKjqBSfqadhf69VehMhwy5 WpZDxU8s+L2SE+L5qaFNCAMl4+hplHuNTiV4xdSexcPOyLctZMkCyOqE1iNhLqGzzs22 vOAmf0BaFIPBP9gUHzlhjOdbLKBxNXH1yz8T9FZ6pTj2Z3tzOqqA21gFGpvKbp7KDgod PwkA== X-Gm-Message-State: AGRZ1gIlUhRx0mmHNPXEgtnKo5sQwK45qG5YPiMeaOeJksJWBNjgZzrR L3Y2BLbMKgpiBO8PFXh1NOkUlaCeMxU= X-Google-Smtp-Source: AJdET5dGK90pJd3v+Wtrpriz2ColV2uqc3Js/l1AHOW0bujut32iJ3CAphgkThFFsXh6wgq/NgW7ug== X-Received: by 2002:a1c:555:: with SMTP id 82-v6mr1337422wmf.37.1540896197796; Tue, 30 Oct 2018 03:43:17 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id q77-v6sm25687339wmd.33.2018.10.30.03.43.16 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 30 Oct 2018 03:43:16 -0700 (PDT) Date: Tue, 30 Oct 2018 10:43:14 +0000 From: Leif Lindholm To: Eric Jin Cc: edk2-devel@lists.01.org, Jiaxin Wu Message-ID: <20181030104314.rbj54dmufo37o6sq@bivouac.eciton.net> References: <20181030083836.7184-1-eric.jin@intel.com> MIME-Version: 1.0 In-Reply-To: <20181030083836.7184-1-eric.jin@intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Assign 0 to the tail of the HwErrRecVariableName 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: Tue, 30 Oct 2018 10:43:20 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Eric, On Tue, Oct 30, 2018 at 04:38:36PM +0800, Eric Jin wrote: > Make the HwErrRecVariableName as valid the string. > Ensure the HwErrRecVariable could be deleted before the test exit. > > Cc: Supreeth Venkatesh (Sidenote - I don't see Supreeth actually on cc on this patch.) > Cc: Jiaxin Wu > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Jin > --- > .../BlackBoxTest/VariableServicesBBTestFunction.c | 12 +++++++----- > .../BlackBoxTest/VariableServicesBBTestMain.h | 10 +++++++++- > 2 files changed, 16 insertions(+), 6 deletions(-) Could you follow the steps outlined in Laszlo's excellent guide https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23 when generating the patches to send out? For this specific patch, this would have made two major differences: 1) We would have seen the full path to files in the summary above, instead of the .../. 2) It would have ordered the files such that the .h would have been shown before the .c. 2 turns out to make a major difference when reviewing this particular patch, because... > diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c > index d1064ce..df1bbe7 100644 > --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c > +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestFunction.c > @@ -1,7 +1,7 @@ > /** @file > > Copyright 2006 - 2012 Unified EFI, Inc.
> - Copyright (c) 2010 - 2012, Intel Corporation. All rights reserved.
> + Copyright (c) 2010 - 2018, 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 > @@ -2855,7 +2855,7 @@ HardwareErrorRecordFuncTest ( > UINT64 RemainingVariableStorageSize; > UINT64 MaximumVariableSize; > > - CHAR16 HwErrRecVariableName[13]; > + CHAR16 HwErrRecVariableName[HwErrRecVariableNameLength]; ... the first thing I see when I look at this patch is "Hang on, are we permitting runtime-sized arrays?" This could have sent me off on a 15-90 minute task of - triple-checking against the coding style document. - search the mailing list (I recalled _some_ discussions around this recently, but that may have been for Linux, and variable-length arrays) - search the edk2 codebase for precedent. - testing with various toolchains whether they were happy compiling it, getting surprised when it worked in some of them, and then spent additional time figuring out why. It could have, but it didn't. Because the patch is small enough that I decided to read through it in its entirety before going off on investigations. More below. > CHAR16 HwErrRecVariable[] = L"This is a HwErrRec variable!"; > > CHAR16 GetVariableName[MAX_BUFFER_SIZE]; > @@ -3015,6 +3015,7 @@ HardwareErrorRecordFuncTest ( > HwErrRecVariableName[0] = L'\0'; > SctStrCat ( HwErrRecVariableName, L"HwErrRec" ); > Myitox( MaxNum, HwErrRecVariableName+8 ); > + HwErrRecVariableName[HwErrRecVariableNameLength-1] = L'\0'; > > // > // Set the new HwErrRec variable to the global variable > @@ -3036,8 +3037,8 @@ HardwareErrorRecordFuncTest ( > // Write reset record > // > RecoveryData[0] = 2; > - SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, 12 ); > - RecoveryLib->WriteResetRecord( RecoveryLib, 13*sizeof(CHAR16)+2, RecoveryData ); > + SctStrnCpy ( (CHAR16*)(&RecoveryData[2]), HwErrRecVariableName, HwErrRecVariableNameLength-1 ); > + RecoveryLib->WriteResetRecord( RecoveryLib, HwErrRecVariableNameLength*sizeof(CHAR16)+2, RecoveryData ); > > // > // Prompt the user about the cold reset and reset the system > @@ -3052,7 +3053,8 @@ HardwareErrorRecordFuncTest ( > // > step2: > DataSize = 255; > - SctStrnCpy ( HwErrRecVariableName, (CHAR16*)(RecoveryData+2), 12 ); > + HwErrRecVariableName[HwErrRecVariableNameLength-1] = L'\0'; > + SctStrnCpy ( HwErrRecVariableName, (CHAR16*)(RecoveryData+2), HwErrRecVariableNameLength-1 ); > Status = RT->GetVariable ( > HwErrRecVariableName, > &gHwErrRecGuid, > diff --git a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h > index 051ae6f..b645b55 100644 > --- a/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h > +++ b/uefi-sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxTest/VariableServicesBBTestMain.h > @@ -1,7 +1,7 @@ > /** @file > > Copyright 2006 - 2016 Unified EFI, Inc.
> - Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
> + Copyright (c) 2010 - 2018, 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 > @@ -125,6 +125,14 @@ Abstract: > #endif > > // > +// The Variable Name of Hardware Error Record Variables > +// defined in the UEFI Spec is HwErrRec####. For example, > +// HwErrRec0001, HwErrRec0002, HwErrRecF31A, etc. > +// Consider the tail of string, the length is 13. > +// > +#define HwErrRecVariableNameLength 13 But then I get here, and I see that HwErrRecVariableNameLength is a statically defined macro. At which point my comment on this patch narrows down to: Please name macros appropriately. HwErrRecVariableNameLength is the name of a variable. The appropriate name for this macro would be HW_ERR_ERC_VARIABLE_NAME_LENGTH. This all according to section 4.3.5 - Type and Macro Names, in the coding standards specification. Regards, Leif