Thanks for the feedback, I'll rename it back to
VariableServiceGetNextVariableInternal ()
and update the comments to refer to FindVariableEx () instead of FindVariable
() in V3.
Thanks,
Michael
> -----Original Message-----
> From: Wu, Hao A <hao.a...@intel.com>
> Sent: Thursday, October 3, 2019 1:03 AM
> To: Kubacki, Michael A <michael.a.kuba...@intel.com>;
> devel@edk2.groups.io
> Cc: Bi, Dandan <dandan...@intel.com>; Ard Biesheuvel
> <ard.biesheu...@linaro.org>; Dong, Eric <eric.d...@intel.com>; Laszlo Ersek
> <ler...@redhat.com>; Gao, Liming <liming....@intel.com>; Kinney, Michael
> D <michael.d.kin...@intel.com>; Ni, Ray <ray...@intel.com>; Wang, Jian J
> <jian.j.w...@intel.com>; Yao, Jiewen <jiewen....@intel.com>
> Subject: RE: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> GetNextVariableEx() store list
>
> A couple of inline comments below:
>
>
> > -----Original Message-----
> > From: Kubacki, Michael A
> > Sent: Saturday, September 28, 2019 9:47 AM
> > To: devel@edk2.groups.io
> > Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming;
> > Kinney, Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> > Subject: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize
> > GetNextVariableEx() store list
> >
> > The majority of logic related to GetNextVariableName () is currently
> > implemented in VariableServiceGetNextVariableInternal (). The list of
> > variable stores to search for the given variable name and variable
> > GUID is defined in the function body. This change renames the function
> > to GetNextVariableEx () since the function is no longer internal to a
> > specific source file and adds a new parameter so that the caller must
> > pass in the list of variable stores to be used in the variable search.
>
>
> I am not sure if 'GetNextVariableEx' is a good name for the function, since:
>
> 1. There is no function named GetNextVariable(), so it is not clear what "Ex"
> means here.
>
> 2. The origin function VariableServiceGetNextVariableInternal() does get
> used
> in multiple source files (Variable.c & VariableExLib.c). I am not sure what
> is the intention for such renaming.
>
>
> >
> > Cc: Dandan Bi <dandan...@intel.com>
> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> > Cc: Eric Dong <eric.d...@intel.com>
> > Cc: Laszlo Ersek <ler...@redhat.com>
> > Cc: Liming Gao <liming....@intel.com>
> > Cc: Michael D Kinney <michael.d.kin...@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Cc: Jian J Wang <jian.j.w...@intel.com>
> > Cc: Hao A Wu <hao.a...@intel.com>
> > Cc: Jiewen Yao <jiewen....@intel.com>
> > Signed-off-by: Michael Kubacki <michael.a.kuba...@intel.com>
> > ---
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h | 15
> ++-
> > -
> > MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 12 ++-
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c | 8 +-
> > MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 78
> > ++++++++++++--------
> > 4 files changed, 73 insertions(+), 40 deletions(-)
> >
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > index 9d77c4916c..0d231511ea 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> > @@ -248,27 +248,30 @@ FindVariableEx (
> > );
> >
> > /**
> > - This code Finds the Next available variable.
> > + This code finds the next available variable.
> >
> > Caution: This function may receive untrusted input.
> > This function may be invoked in SMM mode. This function will do
> > basic validation, before parse the data.
> >
> > - @param[in] VariableName Pointer to variable name.
> > - @param[in] VendorGuid Variable Vendor Guid.
> > - @param[out] VariablePtr Pointer to variable header address.
> > + @param[in] VariableName Pointer to variable name.
> > + @param[in] VendorGuid Variable Vendor Guid.
> > + @param[in] VariableStoreList A list of variable stores that should
> > + be used
> > to get the next variable.
> > + The maximum number of entries is the
> > + max value of
> > VARIABLE_STORE_TYPE.
> > + @param[out] VariablePtr Pointer to variable header address.
> >
> > @retval EFI_SUCCESS The function completed successfully.
> > @retval EFI_NOT_FOUND The next variable was not found.
> > - @retval EFI_INVALID_PARAMETER If VariableName is not an empty
> > string, while VendorGuid is NULL.
> > + @retval EFI_INVALID_PARAMETER If VariableName is nt an empty
> > + string,
> > while VendorGuid is NULL.
> > @retval EFI_INVALID_PARAMETER The input values of VariableName and
> > VendorGuid are not a name and
> > GUID of an existing variable.
> >
> > **/
> > EFI_STATUS
> > EFIAPI
> > -VariableServiceGetNextVariableInternal (
> > +GetNextVariableEx (
> > IN CHAR16 *VariableName,
> > IN EFI_GUID *VendorGuid,
> > + IN VARIABLE_STORE_HEADER **VariableStoreList,
> > OUT VARIABLE_HEADER **VariablePtr
> > );
> >
> > diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > index 76536308e6..816e8f7b8f 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> > @@ -2358,6 +2358,7 @@ VariableServiceGetNextVariableName (
> > UINTN MaxLen;
> > UINTN VarNameSize;
> > VARIABLE_HEADER *VariablePtr;
> > + VARIABLE_STORE_HEADER
> > *VariableStoreHeader[VariableStoreTypeMax];
> >
> > if (VariableNameSize == NULL || VariableName == NULL || VendorGuid
> > ==
> > NULL) {
> > return EFI_INVALID_PARAMETER;
> > @@ -2377,7 +2378,16 @@ VariableServiceGetNextVariableName (
> >
> > AcquireLockOnlyAtBootTime(&mVariableModuleGlobal-
> > >VariableGlobal.VariableServicesLock);
> >
> > - Status = VariableServiceGetNextVariableInternal (VariableName,
> > VendorGuid, &VariablePtr);
> > + //
> > + // 0: Volatile, 1: HOB, 2: Non-Volatile.
> > + // The index and attributes mapping must be kept in this order as
> > FindVariable
> > + // makes use of this mapping to implement search algorithm.
> > + //
> > + VariableStoreHeader[VariableStoreTypeVolatile] =
> > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> > >VariableGlobal.VolatileVariableBase;
> > + VariableStoreHeader[VariableStoreTypeHob] =
> > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> > >VariableGlobal.HobVariableBase;
> > + VariableStoreHeader[VariableStoreTypeNv] = mNvVariableCache;
> > +
> > + Status = GetNextVariableEx (VariableName, VendorGuid,
> > VariableStoreHeader, &VariablePtr);
> > if (!EFI_ERROR (Status)) {
> > VarNameSize = NameSizeOfVariable (VariablePtr);
> > ASSERT (VarNameSize != 0);
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> > index dc78f68fa9..232d9ffe25 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> > @@ -98,10 +98,16 @@ VariableExLibFindNextVariable (
> > EFI_STATUS Status;
> > VARIABLE_HEADER *VariablePtr;
> > AUTHENTICATED_VARIABLE_HEADER *AuthVariablePtr;
> > + VARIABLE_STORE_HEADER
> > *VariableStoreHeader[VariableStoreTypeMax];
> >
> > - Status = VariableServiceGetNextVariableInternal (
> > + VariableStoreHeader[VariableStoreTypeVolatile] =
> > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> > >VariableGlobal.VolatileVariableBase;
> > + VariableStoreHeader[VariableStoreTypeHob] =
> > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> > >VariableGlobal.HobVariableBase;
> > + VariableStoreHeader[VariableStoreTypeNv] = mNvVariableCache;
> > +
> > + Status = GetNextVariableEx (
> > VariableName,
> > VendorGuid,
> > + VariableStoreHeader,
> > &VariablePtr
> > );
> > if (EFI_ERROR (Status)) {
> > diff --git
> > a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> > b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> > index 7de0a90772..9bc5369a90 100644
> > --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> > +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> > @@ -476,14 +476,16 @@ FindVariableEx ( }
> >
> > /**
> > - This code Finds the Next available variable.
> > + This code finds the next available variable.
> >
> > Caution: This function may receive untrusted input.
> > This function may be invoked in SMM mode. This function will do
> > basic validation, before parse the data.
> >
> > - @param[in] VariableName Pointer to variable name.
> > - @param[in] VendorGuid Variable Vendor Guid.
> > - @param[out] VariablePtr Pointer to variable header address.
> > + @param[in] VariableName Pointer to variable name.
> > + @param[in] VendorGuid Variable Vendor Guid.
> > + @param[in] VariableStoreList A list of variable stores that should
> > + be used
> > to get the next variable.
> > + The maximum number of entries is the
> > + max value of
> > VARIABLE_STORE_TYPE.
> > + @param[out] VariablePtr Pointer to variable header address.
> >
> > @retval EFI_SUCCESS The function completed successfully.
> > @retval EFI_NOT_FOUND The next variable was not found.
> > @@ -494,20 +496,41 @@ FindVariableEx ( **/ EFI_STATUS EFIAPI
> > -VariableServiceGetNextVariableInternal (
> > +GetNextVariableEx (
> > IN CHAR16 *VariableName,
> > IN EFI_GUID *VendorGuid,
> > + IN VARIABLE_STORE_HEADER **VariableStoreList,
> > OUT VARIABLE_HEADER **VariablePtr
> > )
> > {
> > - VARIABLE_STORE_TYPE Type;
> > + EFI_STATUS Status;
> > + VARIABLE_STORE_TYPE StoreType;
> > VARIABLE_POINTER_TRACK Variable;
> > VARIABLE_POINTER_TRACK VariableInHob;
> > VARIABLE_POINTER_TRACK VariablePtrTrack;
> > - EFI_STATUS Status;
> > - VARIABLE_STORE_HEADER
> *VariableStoreHeader[VariableStoreTypeMax];
> >
> > - Status = FindVariable (VariableName, VendorGuid, &Variable,
> > &mVariableModuleGlobal->VariableGlobal, FALSE);
> > + Status = EFI_NOT_FOUND;
> > +
> > + if (VariableStoreList == NULL) {
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + // Check if the variable exists in the given variable store list
> > + for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType <
> > VariableStoreTypeMax; StoreType++) {
> > + if (VariableStoreList[StoreType] == NULL) {
> > + continue;
> > + }
> > +
> > + Variable.StartPtr = GetStartPointer (VariableStoreList[StoreType]);
> > + Variable.EndPtr = GetEndPointer (VariableStoreList[StoreType]);
> > + Variable.Volatile = (BOOLEAN) (StoreType ==
> > + VariableStoreTypeVolatile);
> > +
> > + Status = FindVariableEx (VariableName, VendorGuid, FALSE, &Variable);
> > + if (!EFI_ERROR (Status)) {
> > + break;
> > + }
> > + }
> > +
> > if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> > //
> > // For VariableName is an empty string, FindVariable() will try
> > to find and
>
>
> Some description comments within this function that mention
> "FindVariable()"
> can be updated. Since the calling of the FindVariable() has been replaced by
> the above 'for' loop.
>
>
> Best Regards,
> Hao Wu
>
>
> > return
> > @@ -527,39 +550,30 @@ VariableServiceGetNextVariableInternal (
> >
> > if (VariableName[0] != 0) {
> > //
> > - // If variable name is not NULL, get next variable.
> > + // If variable name is not empty, get next variable.
> > //
> > Variable.CurrPtr = GetNextVariablePtr (Variable.CurrPtr);
> > }
> >
> > - //
> > - // 0: Volatile, 1: HOB, 2: Non-Volatile.
> > - // The index and attributes mapping must be kept in this order as
> > FindVariable
> > - // makes use of this mapping to implement search algorithm.
> > - //
> > - VariableStoreHeader[VariableStoreTypeVolatile] =
> > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> > >VariableGlobal.VolatileVariableBase;
> > - VariableStoreHeader[VariableStoreTypeHob] =
> > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> > >VariableGlobal.HobVariableBase;
> > - VariableStoreHeader[VariableStoreTypeNv] = mNvVariableCache;
> > -
> > while (TRUE) {
> > //
> > - // Switch from Volatile to HOB, to Non-Volatile.
> > + // Switch to the next variable store if needed
> > //
> > while (!IsValidVariableHeader (Variable.CurrPtr, Variable.EndPtr)) {
> > //
> > // Find current storage index
> > //
> > - for (Type = (VARIABLE_STORE_TYPE) 0; Type < VariableStoreTypeMax;
> > Type++) {
> > - if ((VariableStoreHeader[Type] != NULL) && (Variable.StartPtr ==
> > GetStartPointer (VariableStoreHeader[Type]))) {
> > + for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType <
> > VariableStoreTypeMax; StoreType++) {
> > + if ((VariableStoreList[StoreType] != NULL) &&
> > + (Variable.StartPtr ==
> > GetStartPointer (VariableStoreList[StoreType]))) {
> > break;
> > }
> > }
> > - ASSERT (Type < VariableStoreTypeMax);
> > + ASSERT (StoreType < VariableStoreTypeMax);
> > //
> > // Switch to next storage
> > //
> > - for (Type++; Type < VariableStoreTypeMax; Type++) {
> > - if (VariableStoreHeader[Type] != NULL) {
> > + for (StoreType++; StoreType < VariableStoreTypeMax; StoreType++) {
> > + if (VariableStoreList[StoreType] != NULL) {
> > break;
> > }
> > }
> > @@ -568,12 +582,12 @@ VariableServiceGetNextVariableInternal (
> > // 1. current storage is the last one, or
> > // 2. no further storage
> > //
> > - if (Type == VariableStoreTypeMax) {
> > + if (StoreType == VariableStoreTypeMax) {
> > Status = EFI_NOT_FOUND;
> > goto Done;
> > }
> > - Variable.StartPtr = GetStartPointer (VariableStoreHeader[Type]);
> > - Variable.EndPtr = GetEndPointer (VariableStoreHeader[Type]);
> > + Variable.StartPtr = GetStartPointer (VariableStoreList[StoreType]);
> > + Variable.EndPtr = GetEndPointer (VariableStoreList[StoreType]);
> > Variable.CurrPtr = Variable.StartPtr;
> > }
> >
> > @@ -605,11 +619,11 @@ VariableServiceGetNextVariableInternal (
> > //
> > // Don't return NV variable when HOB overrides it
> > //
> > - if ((VariableStoreHeader[VariableStoreTypeHob] != NULL) &&
> > (VariableStoreHeader[VariableStoreTypeNv] != NULL) &&
> > - (Variable.StartPtr == GetStartPointer
> > (VariableStoreHeader[VariableStoreTypeNv]))
> > + if ((VariableStoreList[VariableStoreTypeHob] != NULL) &&
> > (VariableStoreList[VariableStoreTypeNv] != NULL) &&
> > + (Variable.StartPtr == GetStartPointer
> > (VariableStoreList[VariableStoreTypeNv]))
> > ) {
> > - VariableInHob.StartPtr = GetStartPointer
> > (VariableStoreHeader[VariableStoreTypeHob]);
> > - VariableInHob.EndPtr = GetEndPointer
> > (VariableStoreHeader[VariableStoreTypeHob]);
> > + VariableInHob.StartPtr = GetStartPointer
> > (VariableStoreList[VariableStoreTypeHob]);
> > + VariableInHob.EndPtr = GetEndPointer
> > (VariableStoreList[VariableStoreTypeHob]);
> > Status = FindVariableEx (
> > GetVariableNamePtr (Variable.CurrPtr),
> > GetVendorGuidPtr (Variable.CurrPtr),
> > --
> > 2.16.2.windows.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48445): https://edk2.groups.io/g/devel/message/48445
Mute This Topic: https://groups.io/mt/34318585/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
