Re: [edk2-devel] [PATCH v2] OvmfPkg: Add QemuFwCfgLibNull

["Laszlo Ersek" <lersek@redhat.com>]

On 04/30/20 15:22, Rebecca Cran wrote:
> Add a null implementation library for QemuFwCfgLib, in order to
> support building PciHostBridgeLib for bhyve.
>
> Signed-off-by: Rebecca Cran <rebecca@bsdio.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  .../Library/QemuFwCfgLib/QemuFwCfgLibNull.inf |  37 ++++
>  OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgNull.c  | 209 ++++++++++++++++++
>  2 files changed, 246 insertions(+)
>  create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibNull.inf
>  create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgNull.c

(a) For the record, the "bhyve v3" instance of this patch [0] and the
"bhyve v4" instance of this patch [1] are identical.

(b) This stand-alone patch differs from both the "bhyve v3" instance [0]
and the (identical) "bhyve v4" instance [1] as follows:

> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgNull.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgNull.c
> index af29471c1f27..2a8f1316ba6c 100644
> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgNull.c
> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgNull.c
> @@ -11,7 +11,7 @@
>
>  #include <Uefi.h>
>  #include <Library/DebugLib.h>
> -#include <IndustryStandard/QemuFwCfg.h>
> +#include <Library/QemuFwCfgLib.h>
>
>  /**
>    Returns a boolean indicating if the firmware configuration interface

which is what Phil asked for in [2]. So that's good.

(c) However, you didn't address my "bhyve v3" comments at all, which I
posted at [3]. (The #include directive that you updated at Phil's
request only happens to solve part of my point (8).)

Please pay more attention to review feedback. If you post a new version
without even attempting to address review comments, that's a waste of
time for both you and your reviewers.

(d) Please get your git setup in functional order *consistently*. This
patch only adds new files (doesn't modify existing files), so there is
no conflict due to <CR><CR><LF> on context lines or removed lines. But
both new files have <CR><CR><LF> after the patch is applied with git-am.

Thanks
Laszlo

[0] http://mid.mail-archive.com/20200421030955.114850-3-rebecca@bsdio.com
    https://edk2.groups.io/g/devel/message/57692

[1] http://mid.mail-archive.com/20200424182914.138915-3-rebecca@bsdio.com
    https://edk2.groups.io/g/devel/message/58098

[2] http://mid.mail-archive.com/5934622c-7235-d073-cdf4-957d6b684c67@redhat.com
    https://edk2.groups.io/g/devel/message/58230

[3] http://mid.mail-archive.com/b3bd046e-1762-243f-e1c6-01660ad6a8d4@redhat.com
    https://edk2.groups.io/g/devel/message/57913