Re: [PATCH] kvm:Return -ENOMEM directly for the function, kvm_create_lapic

Re: [PATCH] kvm:Return -ENOMEM directly for the function, kvm_create_lapic

[Ingo Molnar]

* Nicholas Krause <xerofoify@gmail.com> wrote:

> In order to make code paths easier to read in the function,
> kvm_create_lapic we return -ENOMEM when unable to allocate
> memory for a kvm_lapic structure pointer directly. This
> makes the code easier to read and cleaner then jumping
> to a goto label at the end of the function's body for
> returning just the error code, -ENOMEM.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  arch/x86/kvm/lapic.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 629af0f..88d0cce 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1687,7 +1687,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>  
>  	apic = kzalloc(sizeof(*apic), GFP_KERNEL);
>  	if (!apic)
> -		goto nomem;
> +		return -ENOMEM;
>  
>  	vcpu->arch.apic = apic;
>  
> @@ -1718,7 +1718,6 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>  	return 0;
>  nomem_free_apic:
>  	kfree(apic);
> -nomem:
>  	return -ENOMEM;
>  }

NAK!

You just half destroyed the nice error handling cascade of labels.

Thanks,

	Ingo

Re: [PATCH] kvm:Return -ENOMEM directly for the function, kvm_create_lapic

[Paolo Bonzini]

On 21/05/2015 08:09, Ingo Molnar wrote:
> 
> * Nicholas Krause <xerofoify@gmail.com> wrote:
> 
>> In order to make code paths easier to read in the function,
>> kvm_create_lapic we return -ENOMEM when unable to allocate
>> memory for a kvm_lapic structure pointer directly. This
>> makes the code easier to read and cleaner then jumping
>> to a goto label at the end of the function's body for
>> returning just the error code, -ENOMEM.
>>
>> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
>> ---
>>  arch/x86/kvm/lapic.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 629af0f..88d0cce 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -1687,7 +1687,7 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>>  
>>  	apic = kzalloc(sizeof(*apic), GFP_KERNEL);
>>  	if (!apic)
>> -		goto nomem;
>> +		return -ENOMEM;
>>  
>>  	vcpu->arch.apic = apic;
>>  
>> @@ -1718,7 +1718,6 @@ int kvm_create_lapic(struct kvm_vcpu *vcpu)
>>  	return 0;
>>  nomem_free_apic:
>>  	kfree(apic);
>> -nomem:
>>  	return -ENOMEM;
>>  }
> 
> NAK!
> 
> You just half destroyed the nice error handling cascade of labels.

Right.

What could be done, is always going through kfree(apic), because it is
okay to free NULL.  So the nomem label moves up, and the nomem_free_apic
label is not necessary anymore.

Paolo