Re: [PATCH for-7.1] applesmc: silence invalid key warning in case default one is used

[Daniel P . Berrangé]

On Thu, Jul 28, 2022 at 05:35:58AM -0400, Igor Mammedov wrote:
> QEMU probably can't carry OSK key[1] for legal reasons so it
> can't supply the valid default key. However when tests are run
> applesmc will pollute test log with distracting warning,
> silence that warning so it won't distract maintainers/CI.

What test is causing this problem ?

> 
> If user launches MacOS guest without supplying valid or any key
> they will get upset MacOS (and legality of specifying valid key
> on CLI is also questionable). This patch favors QEMU/tests vs
> MacOS guest.
> 
> A patch from Apple needed to change default to the valid key,
> until then prefer QEMU maintainers sanity/cleaner test logs
> vs any inconvenience users running MacOS guest might suffer.
> 
> 1) After some googling, it seems 'secret' key is the same for
>    all Mac hardware and is widely known but whether it's
>    permited to use that is up to lawyers/Apple to decide.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/misc/applesmc.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/misc/applesmc.c b/hw/misc/applesmc.c
> index 5f9c742e50..bce27b0950 100644
> --- a/hw/misc/applesmc.c
> +++ b/hw/misc/applesmc.c
> @@ -84,7 +84,7 @@ enum {
>  #define smc_debug(...) do { } while (0)
>  #endif
>  
> -static char default_osk[64] = "This is a dummy key. Enter the real key "
> +static char default_osk[65] = "This is a dummy key. Enter the real key "
>                                "using the -osk parameter";

This is a distinct bug fix from the problem described. We should
take this as a standalone fix no matter what.

>  struct AppleSMCData {
> @@ -335,10 +335,12 @@ static void applesmc_isa_realize(DeviceState *dev, 
> Error **errp)
>      isa_register_ioport(&s->parent_obj, &s->io_err,
>                          s->iobase + APPLESMC_ERR_PORT);
>  
> -    if (!s->osk || (strlen(s->osk) != 64)) {
> -        warn_report("Using AppleSMC with invalid key");
> +    if (!s->osk) {
>          s->osk = default_osk;
>      }
> +    if (strlen(s->osk) != 64) {
> +        warn_report("Using AppleSMC with invalid key");
> +    }

This means users who invoke QEMU won't see the warning any more. This
feels like a dubious change, as it degrades the user experiance for
sake of tests - IMHO users takes priority over any testing.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|