Re: [PATCH 4/4] OvmfPkg/QemuVideoDxe: Update QemuVideoDxe driver to bypass NULL pointer detection if enabled.

[Laszlo Ersek <lersek@redhat.com>]

On 09/14/17 03:17, Wang, Jian J wrote:
> Thanks for the comments and good advices. Sorry the format issues.
> This is my first patch for this project. Too many details for me to get 
> familiar with. 
> 
> (1) Sure.
> (2) I'll change that.
> (3) I'll use the tool to ensure the patch format.
> (4) I'll remove the ',' in name
> (5) I'll add more description about it.
> (6) You're right. I should use SetMemorySpaceAttributes() of DXE service
>      instead. The only reason I didn't do it is that I found 
>       GetMemorySpaceDescriptor() doesn't return the same information
>      which SetMemorySpaceAttributes() just changed. So I feel using CPU
>     arch protocol is a bit safer. Anyway, I'll change it.
> (7) I did put those macros in the install function before. To reduce the
>      number of changed files, I made current changes. You're right it's
>      not worthy.
> (8) Using macro can help the readability, which is more important to me.
>     I know function can do the same. But it looks a bit heavy in this situation.
>     I have to admit replacing  the macros with a library is a very good idea,  
>     which brings the same readability. I didn't think of that before. Although 
>     Library is still a little bit heavy to me but it's in a different way, I think it 
>     worth a trying.
> (9) Putting a space before open parenthesis is forced style? If so, I'll add it.

Yes, it is in the CCS:

https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/5_source_files/52_spacing.html#52-spacing

> 5.2.2.6 Always put space before an open parenthesis
>
> The only exception is macro definitions.
>
> if (...
> while (...
> EfiLibAllocateCopyPool (...

Thanks,
Laszlo


> (10) You're right. Using library can reduce the disturbs to affected drivers
>        by this feature to the minimum.