Re: [Qemu-devel] [PATCH 03/11] s390x/migration: Storage attributes device

[Christian Borntraeger]

On 07/13/2017 09:35 AM, Thomas Huth wrote:
> On 13.07.2017 09:11, Christian Borntraeger wrote:
>> Thanks for the review, all valid. Claudio has provided me the following 
>> fixup.
>>
>> I plan to fold that in the base patch (retest pending).
>>
>> Christian
>>
>>
>> ---
>>  hw/s390x/s390-stattrib-kvm.c          | 26 +++++++++++++++++++-------
>>  hw/s390x/s390-stattrib.c              | 12 +++---------
>>  include/hw/s390x/storage-attributes.h |  9 +++++++++
>>  3 files changed, 31 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
>> index 2e7f144..2ab3060 100644
>> --- a/hw/s390x/s390-stattrib-kvm.c
>> +++ b/hw/s390x/s390-stattrib-kvm.c
>> @@ -11,7 +11,6 @@
>>
>>  #include "qemu/osdep.h"
>>  #include "hw/boards.h"
>> -#include "qmp-commands.h"
>>  #include "migration/qemu-file.h"
>>  #include "hw/s390x/storage-attributes.h"
>>  #include "qemu/error-report.h"
>> @@ -19,6 +18,15 @@
>>  #include "exec/ram_addr.h"
>>  #include "cpu.h"
>>
>> +Object *kvm_s390_stattrib_create(void)
>> +{
>> +    if (kvm_enabled() &&
>> +                kvm_check_extension(kvm_state, 
>> KVM_CAP_S390_CMMA_MIGRATION)) {
>> +        return object_new(TYPE_KVM_S390_STATTRIB);
>> +    }
>> +    return object_new(TYPE_QEMU_S390_STATTRIB);
>> +}
> 
> I think you could also return NULL here instead of
> object_new(TYPE_QEMU_S390_STATTRIB) since ...

Yes, I like return NULL better so that we do not create the QEMU one
in *kvm.c. Will also fixup. thanks
> 
>> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
>> index b9533b4..bac9aea 100644
>> --- a/hw/s390x/s390-stattrib.c
>> +++ b/hw/s390x/s390-stattrib.c
>> @@ -40,16 +40,10 @@ void s390_stattrib_init(void)
>>  {
>>      Object *obj;
>>
>> -#ifdef CONFIG_KVM
>> -    if (kvm_enabled() &&
>> -            kvm_check_extension(kvm_state, KVM_CAP_S390_CMMA_MIGRATION)) {
>> -        obj = object_new(TYPE_KVM_S390_STATTRIB);
>> -    } else {
>> +    obj = kvm_s390_stattrib_create();
>> +    if (!obj) {
>>          obj = object_new(TYPE_QEMU_S390_STATTRIB);
>>      }
> 
> ... the object will be created here, too.
> 
>  Thomas
>