Re: [PATCH 1/1] spapr.c: set a 'kvm-type' default value instead of relying on NULL

[Greg Kurz]

On Thu, 10 Dec 2020 10:10:21 -0300
Daniel Henrique Barboza <danielhb413@gmail.com> wrote:

> 
> 
> On 12/10/20 9:47 AM, Greg Kurz wrote:
> > On Thu, 10 Dec 2020 13:34:59 +0100
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> > 
> >> To sum up everything:
> >>
> > 
> > LGTM
> 
> I just sent a v2 with a bit more done (e.g. added ignore case compare
> for 'auto'). Feel free to use that version or this one amended by this
> diff from Paolo.
> 

The "bit more done" in your v2 is very important as it prevents
spapr_kvm_type() to exit(1) if "auto" is explicitly passed on
the command line. So I think we should go ahead with yours.

> 
> Thanks,
> 
> 
> DHB
> 
> > 
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 2d5aeeb45a..61f0963916 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -3028,11 +3028,11 @@ static int spapr_kvm_type(MachineState *machine,
> >> const char *vm_type)
> >>            return 0;
> >>        }
> >>
> >> -    if (!strcmp(vm_type, "HV")) {
> >> +    if (!g_ascii_strcasecmp(vm_type, "hv")) {
> >>            return 1;
> >>        }
> >>
> >> -    if (!strcmp(vm_type, "PR")) {
> >> +    if (!g_ascii_strcasecmp(vm_type, "pr")) {
> >>            return 2;
> >>        }
> >>
> >> @@ -3132,16 +3132,6 @@ static char *spapr_get_kvm_type(Object *obj,
> >> Error **errp)
> >>    {
> >>        SpaprMachineState *spapr = SPAPR_MACHINE(obj);
> >>
> >> -    /*
> >> -     * In case the user didn't set 'kvm-type', return DEFAULT_KVM_TYPE
> >> -     * instead of NULL. This allows us to be more predictable with what
> >> -     * is expected to happen in spapr_kvm_type(), since we can stop 
> >> relying
> >> -     * on receiving a 'NULL' parameter as a valid input there.
> >> -     */
> >> -    if (!spapr->kvm_type) {
> >> -        return g_strdup(DEFAULT_KVM_TYPE);
> >> -    }
> >> -
> >>        return g_strdup(spapr->kvm_type);
> >>    }
> >>
> >> @@ -3294,11 +3284,13 @@ static void spapr_instance_init(Object *obj)
> >>
> >>        spapr->htab_fd = -1;
> >>        spapr->use_hotplug_event_source = true;
> >> +
> >> +    spapr->kvm_type = g_strdup(DEFAULT_KVM_TYPE);
> >>        object_property_add_str(obj, "kvm-type",
> >>                                spapr_get_kvm_type, spapr_set_kvm_type);
> >>        object_property_set_description(obj, "kvm-type",
> >> -                                    "Specifies the KVM virtualization
> >> mode (HV, PR)."
> >> -                                    " If not specified, defaults to any
> >> available KVM"
> >> +                                    "Specifies the KVM virtualization
> >> mode (hv, pr, auto)."
> >> +                                    " auto is the default and allows
> >> any available KVM"
> >>                                        " module loaded in the host. In
> >> case both kvm_hv"
> >>                                        " and kvm_pr are loaded, kvm_hv
> >> takes precedence.");
> >>
> >