On 05/11/2017 09:04 AM, Laszlo Ersek wrote:
I have the following suggestions for this patch: (1) The InternalMemEncryptSevIsEnabled() function is identical between "Ia32/MemEncryptSevLib.c" and "X64/MemEncryptSevLib.c". In addition, that function is the only one exported by "MemEncryptSevLibInternal.h". I suggest to eliminate "MemEncryptSevLibInternal.h", and to move the common implementation of InternalMemEncryptSevIsEnabled() into "MemEncryptSevLibInternal.c". The moved function should be made STATIC. This decreases code duplication and removes an internal header file. (2) Please prefix the names of the extern functions SetMemoryDecrypted() and SetMemoryEncrypted() with "InternalMemEncryptSev". (3) Since this is a BASE library, please don't use EFI_STATUS, EFI_INVALID_PARAMETER, EFI_NO_MAPPING, EFI_SUCCESS; use RETURN_xxx instead. (4) Since this library is going to be linked into multiple modules (presumably), please consider modifying all the debug messages as follows: the format strings should start with "%a: %a: ", and the arguments to pass in should be gEfiCallerBaseName and __FUNCTION__. The former arg will make the library print the BASE_NAME of the containing driver module (from its INF). And __FUNCTION__ helps jumping to the location more quickly.
I was not aware of built-in gEfiCallerBaseName, it will be really handy. I agree with all your comments and will update the patch to remove the code duplication. -Brijesh _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
