Re: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly
On 28 October 2015 at 10:33, Star Zeng wrote: > This patch does below things. > 1. Smbios3Table used as SmbiosTable wrongly after SmbiosTable got from > configuration table. > 2. Correct the return comments of entrypoint function. > 3. Add parameters' comments for MeasureSmbiosTable(). > 4. Remove the tailing space from SmbiosMeasurementDxe.c. > 5. Correct the Protocols and Guids usage comments in SmbiosMeasurementDxe.inf. > 6. Add (VOID **) typecast in LocateProtocol call for GCC build failure. > 7. Use EFI_D_VERBOSE instead of EFI_D_INFO in InternalDumpData() and > InternalDumpHex(). > 8. Use correct VendorGuid and VendorTable to measure. > If you need a numbered list to summarize the changes you are making in a patch, that is usually a strong hint that you should split it into several patches. You have a bug fixes (1,8), comment fixes (2,3,5), a functional change (7), a build fix (6) and a whitespace fix (4), so you should split this into 5 or 6 patches at least. -- Ard. > Cc:Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng > --- > .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.c| 75 > +- > .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.inf | 6 +- > 2 files changed, 46 insertions(+), 35 deletions(-) > > diff --git > a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c > b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c > index 2152827..c10e9f7 100644 > --- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c > +++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c > @@ -1,14 +1,14 @@ > /** @file >This driver measures SMBIOS table to TPM. > - > + > Copyright (c) 2015, 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 > -which accompanies this distribution. The full text of the license may be > found at > -http://opensource.org/licenses/bsd-license.php > - > -THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > -WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +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 > +http://opensource.org/licenses/bsd-license.php > + > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > **/ > > @@ -125,7 +125,7 @@ InternalDumpData ( > { >UINTN Index; >for (Index = 0; Index < Size; Index++) { > -DEBUG ((EFI_D_INFO, "%02x", (UINTN)Data[Index])); > +DEBUG ((EFI_D_VERBOSE, "%02x", (UINTN)Data[Index])); >} > } > > @@ -152,15 +152,15 @@ InternalDumpHex ( >Count = Size / COLUME_SIZE; >Left = Size % COLUME_SIZE; >for (Index = 0; Index < Count; Index++) { > -DEBUG ((EFI_D_INFO, "%04x: ", Index * COLUME_SIZE)); > +DEBUG ((EFI_D_VERBOSE, "%04x: ", Index * COLUME_SIZE)); > InternalDumpData (Data + Index * COLUME_SIZE, COLUME_SIZE); > -DEBUG ((EFI_D_INFO, "\n")); > +DEBUG ((EFI_D_VERBOSE, "\n")); >} > >if (Left != 0) { > -DEBUG ((EFI_D_INFO, "%04x: ", Index * COLUME_SIZE)); > +DEBUG ((EFI_D_VERBOSE, "%04x: ", Index * COLUME_SIZE)); > InternalDumpData (Data + Index * COLUME_SIZE, Left); > -DEBUG ((EFI_D_INFO, "\n")); > +DEBUG ((EFI_D_VERBOSE, "\n")); >} > } > > @@ -217,7 +217,7 @@ GetSmbiosStringById ( >// look for the two consecutive zeros, check the string limit by the way. >// >String = NULL; > - while (*CharInStr != 0 || *(CharInStr+1) != 0) { > + while (*CharInStr != 0 || *(CharInStr+1) != 0) { > if (*CharInStr == 0) { >Size += 1; >CharInStr++; > @@ -271,7 +271,7 @@ FilterSmbiosEntry ( >UINTN StringLen; > >DEBUG ((EFI_D_INFO, "Smbios Table (Type - %d):\n", ((SMBIOS_STRUCTURE > *)TableEntry)->Type)); > - InternalDumpHex (TableEntry, TableEntrySize); > + DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize);); > >FilterStruct = GetFilterStructByType (((SMBIOS_STRUCTURE > *)TableEntry)->Type); >if (FilterStruct != NULL) { > @@ -297,7 +297,7 @@ FilterSmbiosEntry ( >} > >DEBUG ((EFI_D_INFO, "Filter Smbios Table (Type - %d):\n", > ((SMBIOS_STRUCTURE *)TableEntry)->Type)); > - InternalDumpHex (TableEntry, TableEntrySize); > + DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize);); > } > > /** > @@ -306,7 +306,7 @@ FilterSmbiosEntry ( > >@param Head Pointer to the beginning of SMBIOS structure. >@param NumberOfStringsThe returned number of optional strings that > follow the formatted structure. > - > + >@return Size The returned size. > **/ > UINTN > @
Re: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly
Thanks. Can we use CopyGuid() for below assignment? HandoffTables.TableEntry[0].VendorGuid = gEfiSmbiosTableGuid; Reviewed-by: jiewen@intel.com -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Star Zeng Sent: Wednesday, October 28, 2015 9:33 AM To: edk2-devel@lists.01.org Subject: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly This patch does below things. 1. Smbios3Table used as SmbiosTable wrongly after SmbiosTable got from configuration table. 2. Correct the return comments of entrypoint function. 3. Add parameters' comments for MeasureSmbiosTable(). 4. Remove the tailing space from SmbiosMeasurementDxe.c. 5. Correct the Protocols and Guids usage comments in SmbiosMeasurementDxe.inf. 6. Add (VOID **) typecast in LocateProtocol call for GCC build failure. 7. Use EFI_D_VERBOSE instead of EFI_D_INFO in InternalDumpData() and InternalDumpHex(). 8. Use correct VendorGuid and VendorTable to measure. Cc:Jiewen Yao Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Star Zeng --- .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.c| 75 +- .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.inf | 6 +- 2 files changed, 46 insertions(+), 35 deletions(-) diff --git a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c index 2152827..c10e9f7 100644 --- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c +++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c @@ -1,14 +1,14 @@ /** @file This driver measures SMBIOS table to TPM. - + Copyright (c) 2015, 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 -which accompanies this distribution. The full text of the license may be found at -http://opensource.org/licenses/bsd-license.php - -THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, -WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +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 http://opensource.org/licenses/bsd-license.php + +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. **/ @@ -125,7 +125,7 @@ InternalDumpData ( { UINTN Index; for (Index = 0; Index < Size; Index++) { -DEBUG ((EFI_D_INFO, "%02x", (UINTN)Data[Index])); +DEBUG ((EFI_D_VERBOSE, "%02x", (UINTN)Data[Index])); } } @@ -152,15 +152,15 @@ InternalDumpHex ( Count = Size / COLUME_SIZE; Left = Size % COLUME_SIZE; for (Index = 0; Index < Count; Index++) { -DEBUG ((EFI_D_INFO, "%04x: ", Index * COLUME_SIZE)); +DEBUG ((EFI_D_VERBOSE, "%04x: ", Index * COLUME_SIZE)); InternalDumpData (Data + Index * COLUME_SIZE, COLUME_SIZE); -DEBUG ((EFI_D_INFO, "\n")); +DEBUG ((EFI_D_VERBOSE, "\n")); } if (Left != 0) { -DEBUG ((EFI_D_INFO, "%04x: ", Index * COLUME_SIZE)); +DEBUG ((EFI_D_VERBOSE, "%04x: ", Index * COLUME_SIZE)); InternalDumpData (Data + Index * COLUME_SIZE, Left); -DEBUG ((EFI_D_INFO, "\n")); +DEBUG ((EFI_D_VERBOSE, "\n")); } } @@ -217,7 +217,7 @@ GetSmbiosStringById ( // look for the two consecutive zeros, check the string limit by the way. // String = NULL; - while (*CharInStr != 0 || *(CharInStr+1) != 0) { + while (*CharInStr != 0 || *(CharInStr+1) != 0) { if (*CharInStr == 0) { Size += 1; CharInStr++; @@ -271,7 +271,7 @@ FilterSmbiosEntry ( UINTN StringLen; DEBUG ((EFI_D_INFO, "Smbios Table (Type - %d):\n", ((SMBIOS_STRUCTURE *)TableEntry)->Type)); - InternalDumpHex (TableEntry, TableEntrySize); + DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize);); FilterStruct = GetFilterStructByType (((SMBIOS_STRUCTURE *)TableEntry)->Type); if (FilterStruct != NULL) { @@ -297,7 +297,7 @@ FilterSmbiosEntry ( } DEBUG ((EFI_D_INFO, "Filter Smbios Table (Type - %d):\n", ((SMBIOS_STRUCTURE *)TableEntry)->Type)); - InternalDumpHex (TableEntry, TableEntrySize); + DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize);); } /** @@ -306,7 +306,7 @@ FilterSmbiosEntry ( @param Head Pointer to the beginning of SMBIOS structure. @param NumberOfStringsThe returned number of optional strings that follow the formatted structure. - + @return Size
Re: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly
On 2015/10/28 9:44, Ard Biesheuvel wrote: On 28 October 2015 at 10:33, Star Zeng wrote: This patch does below things. 1. Smbios3Table used as SmbiosTable wrongly after SmbiosTable got from configuration table. 2. Correct the return comments of entrypoint function. 3. Add parameters' comments for MeasureSmbiosTable(). 4. Remove the tailing space from SmbiosMeasurementDxe.c. 5. Correct the Protocols and Guids usage comments in SmbiosMeasurementDxe.inf. 6. Add (VOID **) typecast in LocateProtocol call for GCC build failure. 7. Use EFI_D_VERBOSE instead of EFI_D_INFO in InternalDumpData() and InternalDumpHex(). 8. Use correct VendorGuid and VendorTable to measure. If you need a numbered list to summarize the changes you are making in a patch, that is usually a strong hint that you should split it into several patches. You have a bug fixes (1,8), comment fixes (2,3,5), a functional change (7), a build fix (6) and a whitespace fix (4), so you should split this into 5 or 6 patches at least. Good comments. Just followed your suggestion to resend the split patch set. Thanks, Star ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly
On 2015/10/28 10:58, Yao, Jiewen wrote: Thanks. Can we use CopyGuid() for below assignment? HandoffTables.TableEntry[0].VendorGuid = gEfiSmbiosTableGuid; Yes, have included this into PATCH 1/5 of the new split patch set and with your Reviewed-by. Thanks, Star Reviewed-by: jiewen@intel.com -Original Message- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Star Zeng Sent: Wednesday, October 28, 2015 9:33 AM To: edk2-devel@lists.01.org Subject: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly This patch does below things. 1. Smbios3Table used as SmbiosTable wrongly after SmbiosTable got from configuration table. 2. Correct the return comments of entrypoint function. 3. Add parameters' comments for MeasureSmbiosTable(). 4. Remove the tailing space from SmbiosMeasurementDxe.c. 5. Correct the Protocols and Guids usage comments in SmbiosMeasurementDxe.inf. 6. Add (VOID **) typecast in LocateProtocol call for GCC build failure. 7. Use EFI_D_VERBOSE instead of EFI_D_INFO in InternalDumpData() and InternalDumpHex(). 8. Use correct VendorGuid and VendorTable to measure. Cc:Jiewen Yao Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Star Zeng --- .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.c| 75 +- .../SmbiosMeasurementDxe/SmbiosMeasurementDxe.inf | 6 +- 2 files changed, 46 insertions(+), 35 deletions(-) diff --git a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c index 2152827..c10e9f7 100644 --- a/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c +++ b/MdeModulePkg/Universal/SmbiosMeasurementDxe/SmbiosMeasurementDxe.c @@ -1,14 +1,14 @@ /** @file This driver measures SMBIOS table to TPM. - + Copyright (c) 2015, 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 -which accompanies this distribution. The full text of the license may be found at -http://opensource.org/licenses/bsd-license.php - -THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, -WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +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 http://opensource.org/licenses/bsd-license.php + +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. **/ @@ -125,7 +125,7 @@ InternalDumpData ( { UINTN Index; for (Index = 0; Index < Size; Index++) { -DEBUG ((EFI_D_INFO, "%02x", (UINTN)Data[Index])); +DEBUG ((EFI_D_VERBOSE, "%02x", (UINTN)Data[Index])); } } @@ -152,15 +152,15 @@ InternalDumpHex ( Count = Size / COLUME_SIZE; Left = Size % COLUME_SIZE; for (Index = 0; Index < Count; Index++) { -DEBUG ((EFI_D_INFO, "%04x: ", Index * COLUME_SIZE)); +DEBUG ((EFI_D_VERBOSE, "%04x: ", Index * COLUME_SIZE)); InternalDumpData (Data + Index * COLUME_SIZE, COLUME_SIZE); -DEBUG ((EFI_D_INFO, "\n")); +DEBUG ((EFI_D_VERBOSE, "\n")); } if (Left != 0) { -DEBUG ((EFI_D_INFO, "%04x: ", Index * COLUME_SIZE)); +DEBUG ((EFI_D_VERBOSE, "%04x: ", Index * COLUME_SIZE)); InternalDumpData (Data + Index * COLUME_SIZE, Left); -DEBUG ((EFI_D_INFO, "\n")); +DEBUG ((EFI_D_VERBOSE, "\n")); } } @@ -217,7 +217,7 @@ GetSmbiosStringById ( // look for the two consecutive zeros, check the string limit by the way. // String = NULL; - while (*CharInStr != 0 || *(CharInStr+1) != 0) { + while (*CharInStr != 0 || *(CharInStr+1) != 0) { if (*CharInStr == 0) { Size += 1; CharInStr++; @@ -271,7 +271,7 @@ FilterSmbiosEntry ( UINTN StringLen; DEBUG ((EFI_D_INFO, "Smbios Table (Type - %d):\n", ((SMBIOS_STRUCTURE *)TableEntry)->Type)); - InternalDumpHex (TableEntry, TableEntrySize); + DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize);); FilterStruct = GetFilterStructByType (((SMBIOS_STRUCTURE *)TableEntry)->Type); if (FilterStruct != NULL) { @@ -297,7 +297,7 @@ FilterSmbiosEntry ( } DEBUG ((EFI_D_INFO, "Filter Smbios Table (Type - %d):\n", ((SMBIOS_STRUCTURE *)TableEntry)->Type)); - InternalDumpHex (TableEntry, TableEntrySize); + DEBUG_CODE (InternalDumpHex (TableEntry, TableEntrySize);); } /** @@ -306,7 +306,7 @@ FilterSmbiosEntry ( @param Head Pointer to the beginning of SMBIOS structure. @param NumberOfStringsThe returned number of optional strings that follow the formatted structure. - + @return Size The returned size. **/ UINTN @@ -3
Re: [edk2] [PATCH] MdeModulePkg SmbiosMeasurementDxe: Smbios3Table used as SmbiosTable wrongly
On 28 October 2015 at 13:26, Zeng, Star wrote: > On 2015/10/28 9:44, Ard Biesheuvel wrote: >> >> On 28 October 2015 at 10:33, Star Zeng wrote: >>> >>> This patch does below things. >>> 1. Smbios3Table used as SmbiosTable wrongly after SmbiosTable got from >>> configuration table. >>> 2. Correct the return comments of entrypoint function. >>> 3. Add parameters' comments for MeasureSmbiosTable(). >>> 4. Remove the tailing space from SmbiosMeasurementDxe.c. >>> 5. Correct the Protocols and Guids usage comments in >>> SmbiosMeasurementDxe.inf. >>> 6. Add (VOID **) typecast in LocateProtocol call for GCC build failure. >>> 7. Use EFI_D_VERBOSE instead of EFI_D_INFO in InternalDumpData() and >>> InternalDumpHex(). >>> 8. Use correct VendorGuid and VendorTable to measure. >>> >> >> If you need a numbered list to summarize the changes you are making in >> a patch, that is usually a strong hint that you should split it into >> several patches. You have a bug fixes (1,8), comment fixes (2,3,5), a >> functional change (7), a build fix (6) and a whitespace fix (4), so >> you should split this into 5 or 6 patches at least. > > > Good comments. > Just followed your suggestion to resend the split patch set. > Thank you, Star! ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel