Re: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

["Zeng, Star" <star.zeng@intel.com>]

Ray,

The code is like low hanging fruit from my practice for me, and I don't think it hurts readability although it may not bring performance improvement, it depends on how many variables in variable region, how many times of calling GetNextVariableName, and how fast of GetNextVariableName.

The code practice I did is on NT32 and my real platforms. Is there anyone can make sure he/she tested all the systems in the world for their code?


Anyway, I can update the patch if you insist.


Thanks,
Star
-----Original Message-----
From: Ni, Ruiyu 
Sent: Saturday, June 24, 2017 10:08 AM
To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
Cc: Gao, Liming <liming.gao@intel.com>
Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update GetNextVariableName to follow UEFI 2.7

Star,
I don't recommend to add the additional check for performance consideration.
Because we have no idea what the input VariableName buffer is like.
If the VariableName is like ['\0', '?', '?', '?'] with MaxLen equals to 4, "VariableName[MaxLen-1] != 0" check is redundant.
The NT32 case you met cannot represent the all possible cases.
You could use the possibility theory to decide what the most efficient way is.

Additionally I think code readability is more important than efficiency.
In this case, we need the data about the performance improvement to decide whether this check is necessary.


Regards,
Ray

>-----Original Message-----
>From: Zeng, Star
>Sent: Friday, June 23, 2017 5:33 PM
>To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; Zeng, Star 
><star.zeng@intel.com>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
>GetNextVariableName to follow UEFI 2.7
>
>Ray,
>
>It is to pass the check quickly and avoid scanning all the chars in 
>VariableName by StrnLenS for normal boot without invalid cases.
>I did experiments in the code of GetNextVariableName with below debug 
>code for normal boot on NT32 and my real platforms, all the cases will go into the branch "xxx 2".
>  if (((VariableName[MaxLen - 1] != 0))) {
>    DEBUG ((DEBUG_INFO, "xxx 1\n"));
>  } else {
>    DEBUG ((DEBUG_INFO, "xxx 2\n"));
>  }
>
>
>Thanks,
>Star
>-----Original Message-----
>From: Ni, Ruiyu
>Sent: Friday, June 23, 2017 4:20 PM
>To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>
>Subject: RE: [PATCH V2 3/3] DuetPkg FsVariable: Update 
>GetNextVariableName to follow UEFI 2.7
>
>Star,
>What's the benefit of this check "VariableName[MaxLen - 1] != 0"?
>I think this check "StrnLenS (VariableName, MaxLen) == MaxLen" should be enough.
>
>Thanks/Ray
>
>> -----Original Message-----
>> From: Zeng, Star
>> Sent: Friday, June 23, 2017 4:08 PM
>> To: edk2-devel@lists.01.org
>> Cc: Zeng, Star <star.zeng@intel.com>; Gao, Liming 
>> <liming.gao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
>> Subject: [PATCH V2 3/3] DuetPkg FsVariable: Update 
>> GetNextVariableName to follow UEFI 2.7
>>
>> "The size must be large enough to fit input string supplied in 
>> VariableName buffer" is added in the description for VariableNameSize.
>> And two cases of EFI_INVALID_PARAMETER are added.
>> 1. The input values of VariableName and VendorGuid are not a name and
>>    GUID of an existing variable.
>> 2. Null-terminator is not found in the first VariableNameSize bytes of
>>    the input VariableName buffer.
>>
>> This patch is to update code to follow them.
>>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Star Zeng <star.zeng@intel.com>
>> ---
>>  DuetPkg/FSVariable/FSVariable.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/DuetPkg/FSVariable/FSVariable.c 
>> b/DuetPkg/FSVariable/FSVariable.c index 34b79305c871..6069cfa8fb98 
>> 100644
>> --- a/DuetPkg/FSVariable/FSVariable.c
>> +++ b/DuetPkg/FSVariable/FSVariable.c
>> @@ -6,7 +6,7 @@ disk. They can be changed by user. BIOS is not able 
>> to protoect those.
>>  Duet trusts all meta data from disk. If variable code, variable 
>> metadata and variable  data is modified in inproper way, the behavior 
>> is undefined.
>>
>> -Copyright (c) 2006 - 2016, Intel Corporation. All rights 
>> reserved.<BR>
>> +Copyright (c) 2006 - 2017, Intel Corporation. All rights 
>> +reserved.<BR>
>>  This program and the accompanying materials  are licensed and made 
>> available under the terms and conditions of the BSD License  which 
>> accompanies this distribution.  The full text of the license may be 
>> found at @@ -1400,14 +1400,33 @@ Returns:
>>    VARIABLE_POINTER_TRACK  Variable;
>>    UINTN                   VarNameSize;
>>    EFI_STATUS              Status;
>> +  UINTN                   MaxLen;
>>
>>    if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
>> NULL) {
>>      return EFI_INVALID_PARAMETER;
>>    }
>>
>> +  //
>> +  // Calculate the possible maximum length of name string, including the Null
>> terminator.
>> +  //
>> +  MaxLen = *VariableNameSize / sizeof (CHAR16);
>> +  if ((MaxLen == 0) ||
>> +      ((VariableName[MaxLen - 1] != 0) && (StrnLenS (VariableName, MaxLen)
>> == MaxLen))) {
>> +    //
>> +    // Null-terminator is not found in the first VariableNameSize bytes of the
>> input VariableName buffer.
>> +    //
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
>>    Status = FindVariable (VariableName, VendorGuid, &Variable);
>>
>>    if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
>> +    if (VariableName[0] != 0) {
>> +      //
>> +      // The input values of VariableName and VendorGuid are not a name
>> and GUID of an existing variable.
>> +      //
>> +      Status = EFI_INVALID_PARAMETER;
>> +    }
>>      return Status;
>>    }
>>
>> --
>> 2.7.0.windows.1