Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
> -Original Message-
> From: Wei Yang [mailto:richardw.y...@linux.intel.com]
> Sent: Monday, August 19, 2019 10:39 AM
> To: David Hildenbrand
> Cc: Wei Yang ; Zeng, Star
> ; imamm...@redhat.com; qemu-devel@nongnu.org;
> m...@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
>
> On Thu, Aug 08, 2019 at 09:06:21AM +0200, David Hildenbrand wrote:
> >On 08.08.19 04:38, Wei Yang wrote:
> >> On Thu, Aug 08, 2019 at 02:30:02AM +, Zeng, Star wrote:
> >>>> -Original Message-
> >>>> From: Wei Yang [mailto:richardw.y...@linux.intel.com]
> >>>> Sent: Thursday, August 8, 2019 10:13 AM
> >>>> To: Zeng, Star
> >>>> Cc: Wei Yang ;
> >>>> qemu-devel@nongnu.org; imamm...@redhat.com;
> da...@redhat.com;
> >>>> m...@redhat.com
> >>>> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not
> >>>> necessary to use goto for the last check
> >>>>
> >>>> On Thu, Aug 08, 2019 at 01:42:14AM +, Zeng, Star wrote:
> >>>>>> -Original Message-
> >>>>>> From: Qemu-devel [mailto:qemu-devel-
> >>>>>> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
> >>>>>> Sent: Tuesday, July 30, 2019 8:38 AM
> >>>>>> To: qemu-devel@nongnu.org
> >>>>>> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
> >>>>>> ; m...@redhat.com
> >>>>>> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not
> necessary
> >>>>>> to use goto for the last check
> >>>>>>
> >>>>>> We are already at the last condition check.
> >>>>>>
> >>>>>> Signed-off-by: Wei Yang
> >>>>>> Reviewed-by: Igor Mammedov
> >>>>>> Reviewed-by: David Hildenbrand
> >>>>>> ---
> >>>>>> hw/mem/memory-device.c | 1 -
> >>>>>> 1 file changed, 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-
> device.c
> >>>> index
> >>>>>> 5f2c408036..df3261b32a 100644
> >>>>>> --- a/hw/mem/memory-device.c
> >>>>>> +++ b/hw/mem/memory-device.c
> >>>>>> @@ -186,7 +186,6 @@ static uint64_t
> >>>>>> memory_device_get_free_addr(MachineState *ms,
> >>>>>> if (!range_contains_range(, )) {
> >>>>>> error_setg(errp, "could not find position in guest address
> >>>>>> space
> for "
> >>>>>> "memory device - memory fragmented due to
> alignments");
> >>>>>> -goto out;
> >>>>>
> >>>>> Is it better to return 0 (or set new_addr to 0) for this error
> >>>>> path and another
> >>>> remaining "goto out" path?
> >>>>>
> >>>>
> >>>> I may not get your point.
> >>>>
> >>>> We set errp which is handled in its caller. By doing so, the error is
> propagated.
> >>>>
> >>>> Do I miss something?
> >>>
> >>> Yes, you are right. Currently, the caller is checking errp, but not the
> returned address, so there should be no issue.
> >>> But when you see other error paths, you will find they all return 0.
> >>> To be aligned (return 0 when error), so just suggest also returning
> >>> 0 for these two "goto out" error path. :)
> >>>
> >>
> >> You may have some point.
> >>
> >> Let's see whether others have the same taste, or we can refine it
> separately.
> >>
> >
> >I don't think that's necessary (callers really should check for errors
> >before using the return values), but I would also not object to that change.
> >
>
> In case there is no strong requirement to refactor the code. I would leave it
> here.
It was just my suggestion. I am fine with any preference you and other experts
have.
Thanks,
Star
>
> >--
> >
> >Thanks,
> >
> >David / dhildenb
>
> --
> Wei Yang
> Help you, Help me
Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
> -Original Message-
> From: Wei Yang [mailto:richardw.y...@linux.intel.com]
> Sent: Thursday, August 8, 2019 10:13 AM
> To: Zeng, Star
> Cc: Wei Yang ; qemu-devel@nongnu.org;
> imamm...@redhat.com; da...@redhat.com; m...@redhat.com
> Subject: Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> use goto for the last check
>
> On Thu, Aug 08, 2019 at 01:42:14AM +, Zeng, Star wrote:
> >> -Original Message-
> >> From: Qemu-devel [mailto:qemu-devel-
> >> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
> >> Sent: Tuesday, July 30, 2019 8:38 AM
> >> To: qemu-devel@nongnu.org
> >> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
> >> ; m...@redhat.com
> >> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to
> >> use goto for the last check
> >>
> >> We are already at the last condition check.
> >>
> >> Signed-off-by: Wei Yang
> >> Reviewed-by: Igor Mammedov
> >> Reviewed-by: David Hildenbrand
> >> ---
> >> hw/mem/memory-device.c | 1 -
> >> 1 file changed, 1 deletion(-)
> >>
> >> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index
> >> 5f2c408036..df3261b32a 100644
> >> --- a/hw/mem/memory-device.c
> >> +++ b/hw/mem/memory-device.c
> >> @@ -186,7 +186,6 @@ static uint64_t
> >> memory_device_get_free_addr(MachineState *ms,
> >> if (!range_contains_range(, )) {
> >> error_setg(errp, "could not find position in guest address space
> >> for "
> >> "memory device - memory fragmented due to alignments");
> >> -goto out;
> >
> >Is it better to return 0 (or set new_addr to 0) for this error path and
> >another
> remaining "goto out" path?
> >
>
> I may not get your point.
>
> We set errp which is handled in its caller. By doing so, the error is
> propagated.
>
> Do I miss something?
Yes, you are right. Currently, the caller is checking errp, but not the
returned address, so there should be no issue.
But when you see other error paths, you will find they all return 0. To be
aligned (return 0 when error), so just suggest also returning 0 for these two
"goto out" error path. :)
Thanks,
Star
>
> >
> >Thanks,
> >Star
> >
> >> }
> >> out:
> >> g_slist_free(list);
> >> --
> >> 2.17.1
> >>
>
> --
> Wei Yang
> Help you, Help me
Re: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use goto for the last check
> -Original Message-
> From: Qemu-devel [mailto:qemu-devel-
> bounces+star.zeng=intel@nongnu.org] On Behalf Of Wei Yang
> Sent: Tuesday, July 30, 2019 8:38 AM
> To: qemu-devel@nongnu.org
> Cc: imamm...@redhat.com; da...@redhat.com; Wei Yang
> ; m...@redhat.com
> Subject: [Qemu-devel] [PATCH v2 1/2] memory-device: not necessary to use
> goto for the last check
>
> We are already at the last condition check.
>
> Signed-off-by: Wei Yang
> Reviewed-by: Igor Mammedov
> Reviewed-by: David Hildenbrand
> ---
> hw/mem/memory-device.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c index
> 5f2c408036..df3261b32a 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -186,7 +186,6 @@ static uint64_t
> memory_device_get_free_addr(MachineState *ms,
> if (!range_contains_range(, )) {
> error_setg(errp, "could not find position in guest address space for
> "
> "memory device - memory fragmented due to alignments");
> -goto out;
Is it better to return 0 (or set new_addr to 0) for this error path and another
remaining "goto out" path?
Thanks,
Star
> }
> out:
> g_slist_free(list);
> --
> 2.17.1
>
Re: [Qemu-devel] [edk2] [PATCH v2 3/8] MdeModulePkg: fix REGISITER -> REGISTER
I agree with Laszlo's suggestion.
And it is good observation.
Reviewed-by: Star Zeng <star.z...@intel.com>
Thanks,
Star
-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Thursday, March 8, 2018 7:59 PM
To: marcandre.lur...@redhat.com; edk2-de...@lists.01.org
Cc: qemu-devel@nongnu.org; javi...@redhat.com; pjo...@redhat.com; Yao, Jiewen
<jiewen@intel.com>; Zeng, Star <star.z...@intel.com>; Dong, Eric
<eric.d...@intel.com>
Subject: Re: [edk2] [PATCH v2 3/8] MdeModulePkg: fix REGISITER -> REGISTER
On 03/07/18 16:57, marcandre.lur...@redhat.com wrote:
> From: Marc-André Lureau <marcandre.lur...@redhat.com>
>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com>
> ---
> MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 18 +-
> MdeModulePkg/Core/Pei/Image/Image.c | 4 ++--
> MdeModulePkg/Core/Pei/PeiMain.h | 2 +-
> MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 2 +-
> 4 files changed, 13 insertions(+), 13 deletions(-)
CC'ing Star & Eric (see Maintainers.txt).
I suggest changing te subject like this:
MdeModulePkg/Core/Pei: fix REGISITER -> REGISTER typo
And just so the commit message isn't empty, let's say there, "No functional
changes.".
With those updates:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks
Laszlo
> diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> index 79f2e5cebcbe..027176d872c7 100644
> --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c
> @@ -970,7 +970,7 @@ PeiDispatcher (
>if ((Private->PeiMemoryInstalled) &&
> (Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME ||
> PcdGetBool (PcdShadowPeimOnS3Boot))) {
> //
> // Once real memory is available, shadow the RegisterForShadow modules.
> And meanwhile
> -// update the modules' status from PEIM_STATE_REGISITER_FOR_SHADOW to
> PEIM_STATE_DONE.
> +// update the modules' status from PEIM_STATE_REGISTER_FOR_SHADOW to
> PEIM_STATE_DONE.
> //
> SaveCurrentPeimCount = Private->CurrentPeimCount;
> SaveCurrentFvCount= Private->CurrentPeimFvCount;
> @@ -978,7 +978,7 @@ PeiDispatcher (
>
> for (Index1 = 0; Index1 <= SaveCurrentFvCount; Index1++) {
>for (Index2 = 0; (Index2 < PcdGet32 (PcdPeiCoreMaxPeimPerFv)) &&
> (Private->Fv[Index1].FvFileHandles[Index2] != NULL); Index2++) {
> -if (Private->Fv[Index1].PeimState[Index2] ==
> PEIM_STATE_REGISITER_FOR_SHADOW) {
> +if (Private->Fv[Index1].PeimState[Index2] ==
> + PEIM_STATE_REGISTER_FOR_SHADOW) {
>PeimFileHandle = Private->Fv[Index1].FvFileHandles[Index2];
>Private->CurrentFileHandle = PeimFileHandle;
>Private->CurrentPeimFvCount = Index1; @@ -986,13 +986,13
> @@ PeiDispatcher (
>Status = PeiLoadImage (
> (CONST EFI_PEI_SERVICES **) >Ps,
> PeimFileHandle,
> -PEIM_STATE_REGISITER_FOR_SHADOW,
> +PEIM_STATE_REGISTER_FOR_SHADOW,
> ,
>
> );
>if (Status == EFI_SUCCESS) {
> //
> -// PEIM_STATE_REGISITER_FOR_SHADOW move to PEIM_STATE_DONE
> +// PEIM_STATE_REGISTER_FOR_SHADOW move to PEIM_STATE_DONE
> //
> Private->Fv[Index1].PeimState[Index2]++;
> //
> @@ -1165,7 +1165,7 @@ PeiDispatcher (
> //
> PeiCheckAndSwitchStack (SecCoreData, Private);
>
> -if ((Private->PeiMemoryInstalled) &&
> (Private->Fv[FvCount].PeimState[PeimCount] ==
> PEIM_STATE_REGISITER_FOR_SHADOW) && \
> +if ((Private->PeiMemoryInstalled) &&
> (Private->Fv[FvCount].PeimState[PeimCount] == PEIM_STATE_REGISTER_FOR_SHADOW)
> && \
> (Private->HobList.HandoffInformationTable->BootMode !=
> BOOT_ON_S3_RESUME || PcdGetBool (PcdShadowPeimOnS3Boot))) {
>//
>// If memory is available we shadow images by default for
> performance reasons.
> @@ -1179,7 +1179,7 @@ PeiDispatcher (
> Status = PeiLoadImage (
> PeiServices,
> PeimFileHandle,
> - PEIM_STATE_REGISITER_FOR_SHADOW,
> + PE
Re: [Qemu-devel] [PATCH v2 1/8] SecurityPkg: also clear HashInterfaceHob.SupportedHashMask
Reviewed-by: Star Zeng <star.z...@intel.com> Thanks, Star -Original Message- From: Zhang, Chao B Sent: Thursday, March 8, 2018 8:35 AM To: marcandre.lur...@redhat.com; edk2-de...@lists.01.org Cc: pjo...@redhat.com; Yao, Jiewen <jiewen@intel.com>; stef...@linux.vnet.ibm.com; ler...@redhat.com; qemu-devel@nongnu.org; javi...@redhat.com; Zeng, Star <star.z...@intel.com> Subject: RE: [PATCH v2 1/8] SecurityPkg: also clear HashInterfaceHob.SupportedHashMask Reviewed-by: Chao Zhang<chao.b.zh...@intel.com> -Original Message- From: marcandre.lur...@redhat.com [mailto:marcandre.lur...@redhat.com] Sent: Wednesday, March 7, 2018 11:58 PM To: edk2-de...@lists.01.org Cc: pjo...@redhat.com; Yao, Jiewen <jiewen@intel.com>; stef...@linux.vnet.ibm.com; ler...@redhat.com; qemu-devel@nongnu.org; javi...@redhat.com; Marc-André Lureau <marcandre.lur...@redhat.com>; Zhang, Chao B <chao.b.zh...@intel.com>; Zeng, Star <star.z...@intel.com> Subject: [PATCH v2 1/8] SecurityPkg: also clear HashInterfaceHob.SupportedHashMask From: Marc-André Lureau <marcandre.lur...@redhat.com> Commit 4cc2b63bd829426b05bad0d8952f1855a10d6ed7 fixed an out of bounds ZeroMem() call. However, as Laszlo Ersek pointed out, the intent was to clear all but the Identifier (to revert the effect of RegisterHashInterfaceLib()). For that, it should clear the SupportedHashMask too. Cc: Jiewen Yao <jiewen@intel.com> Cc: Chao Zhang <chao.b.zh...@intel.com> Cc: Star Zeng <star.z...@intel.com> Cc: Laszlo Ersek <ler...@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> --- .../Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c | 1 + 1 file changed, 1 insertion(+) diff --git a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c index 361a4f6508a0..bf6e1336ee76 100644 --- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterPei.c +++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRoute +++ rPei.c @@ -426,6 +426,7 @@ HashLibBaseCryptoRouterPeiConstructor ( // ZeroMem (>HashInterface, sizeof (HashInterfaceHob->HashInterface)); HashInterfaceHob->HashInterfaceCount = 0; +HashInterfaceHob->SupportedHashMask = 0; } // -- 2.16.2.346.g9779355e34
