On 06/01/15 14:08, Heyi Guo wrote:
> Add HiiIsConfigHdrMatch check in RouteConfig, or the test case in
> UEFI SCT will fail for Configuration with invalid GUID, because it
> expects the return value to be NOT FOUND but actually INVALID
> PARAMETER returned.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi....@linaro.org>
> ---
> OvmfPkg/PlatformDxe/Platform.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
> index 35fabf8..48a24fd 100644
> --- a/OvmfPkg/PlatformDxe/Platform.c
> +++ b/OvmfPkg/PlatformDxe/Platform.c
> @@ -340,6 +340,11 @@ RouteConfig (
> DEBUG ((EFI_D_VERBOSE, "%a: Configuration=\"%s\"\n", __FUNCTION__,
> Configuration));
>
> + if (!HiiIsConfigHdrMatch (Configuration, &gOvmfPlatformConfigGuid, NULL)) {
> + *Progress = Configuration;
> + return EFI_NOT_FOUND;
> + }
> +
> //
> // the "read" step in RMW
> //
>
The SCT happens to be correct in catching this error.
However, I think the error is not in OVMF, but in the
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() implementation (exactly
as you say, it returns EFI_INVALID_PARAMETER, which is not correct).
Check the spec on EFI_HII_CONFIG_ACCESS_PROTOCOL.RouteConfig():
EFI_NOT_FOUND
Target for the specified routing data was not found
Okay; from a little higher:
If the driver's configuration is stored in a linear block of data and
the driver's name / value pairs are in <BlockConfig> format, it may
use the ConfigToBlock helper function (above) to simplify the job.
That's what we have here, hence the call to
gHiiConfigRouting->ConfigToBlock().
Then EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() says:
EFI_NOT_FOUND
Target for the specified routing data was not found. Progress
points to the “G” in “GUID” of the errant routing data.
So, if gHiiConfigRouting->ConfigToBlock() works okay, then this call in
RouteConfig() will satisfy the spec (and the SCT too).
Unfortunately, as you say, the implementation of
EFI_HII_CONFIG_ROUTING_PROTOCOL.ConfigToBlock() is not correct in this
regard.
Namely, I checked HiiConfigToBlock() in
"MdeModulePkg/Universal/HiiDatabaseDxe/ConfigRouting.c". Its leading
comment documents EFI_NOT_FOUND, but the function never seems to return
EFI_NOT_FOUND. In fact it doesn't even try to *determine* if the GUID is
a match or not; it just skips over the GUID. I don't know how that could
be fixed easily -- maybe it's another issue with the UEFI spec.
Then I grepped the edk2 source code for invocations of
HiiIsConfigHdrMatch(). That's when I realized two things:
- most of the functions I peeked at use the same style error checks that
your patch 2/3 does.
- your HiiIsConfigHdrMatch() call seems to match existing practice
(based on other RouteConfig() implementations), but I would also like to
see the third parameter filled in, with L"MainFormState" rather than
NULL. Can you please test that?
In fact, seeing how your earlier patches actually just followed existing
edk2 practices is a *huge* disappointment for me (about edk2, not your
patchset). In any case, it's lucky for you, because I've stopped caring.
So please do the following:
- Please review the commit messages on the patches, and adapt the
language to state "work around" or "paper over" or "suppress" invalid
SCT test errors. *Unless* you find a specific violation of the UEFI-2.5
spec, of course, in which case please spell out those locations
individually.
- For all new exit conditions and/or error values introduced, please
document them in each function's leading comment. There's no need to
over-emphasize it, but please be clear about the fact that these checks
/ retvals are being done for consistency with the rest of the edk2
codebase and/or due to SCT bugs, and not for UEFI spec conformance.
(Unless that's the case.)
- Please open-code the L"MainFormState" CHAR16 string as the third
argument of the HiiIsConfigHdrMatch() call in patch #3 (and please test
it too, with OVMF as well -- see if it remains possible to change the
preferred resolution).
With those changes I'll accept your patches, grudgingly.
This "paper over broken tools" has been going on since forever in OVMF,
the most common example being the *invalid* warning messages emitted by
various MSVC compilers, breaking the build for no good reason. Now the
SCT is being added to that list. I guess I'll just have to accept this
(very demoralizing) status quo.
Thanks
Laszlo
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel
