Re: [Qemu-devel] [PATCH] pc-dimm: use same mechanism for [get|set]_addr

2019-02-07 Thread Igor Mammedov 
On Wed, 6 Feb 2019 23:55:15 +
Wei Yang  wrote:

> On Wed, Feb 06, 2019 at 12:59:49PM +0100, Igor Mammedov wrote:
> >On Fri,  1 Feb 2019 09:08:37 +0800
> >Wei Yang  wrote:
> >
> >> [get|set]_addr are two counterpart to access PCDIMMDevice.addr.
> >> 
> >> Since we have already set up a property PC_DIMM_ADDR_PROP for this
> >> field and use this mechanism in set_addr, it would be more proper to use
> >> the same mechanism in set_addr.
> >> 
> >> This patch uses object_property_get_uint() to replace the direct memory
> >> access to make [get|set]_addr with the same mechanism.
> >> 
> >> Signed-off-by: Wei Yang 
> >> ---
> >>  hw/mem/pc-dimm.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >> 
> >> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> >> index 0c9b9e8292..c0658b9b88 100644
> >> --- a/hw/mem/pc-dimm.c
> >> +++ b/hw/mem/pc-dimm.c
> >> @@ -204,9 +204,7 @@ static MemoryRegion 
> >> *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
> >>  
> >>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
> >>  {
> >> -const PCDIMMDevice *dimm = PC_DIMM(md);
> >> -
> >> -return dimm->addr;
> >> +return object_property_get_uint(OBJECT(md), PC_DIMM_ADDR_PROP, NULL);
> >it's not good to ignore errors
> >
> >s/NULL/error_abort/
> >
> >so we would notice if error ever happened.
> 
> You mean something like this?
> 
>   Error *err;
>   uint64_t addr;
> 
>   addr = object_property_get_uint(OBJECT(md), PC_DIMM_ADDR_PROP, );

see how error_abort is used in QEMU,
point is if there is a error that should never happen it is programming error so
QEMU should crash and we should fix it.

> 
>   if (err) {
>   warn_report_err();
>   return 0;
>   }
> 
>   return addr;
> 
> >
> >>  }
> >>  
> >>  static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
> >
>

Re: [Qemu-devel] [PATCH] pc-dimm: use same mechanism for [get|set]_addr

2019-02-06 Thread Wei Yang 
On Wed, Feb 06, 2019 at 12:59:49PM +0100, Igor Mammedov wrote:
>On Fri,  1 Feb 2019 09:08:37 +0800
>Wei Yang  wrote:
>
>> [get|set]_addr are two counterpart to access PCDIMMDevice.addr.
>> 
>> Since we have already set up a property PC_DIMM_ADDR_PROP for this
>> field and use this mechanism in set_addr, it would be more proper to use
>> the same mechanism in set_addr.
>> 
>> This patch uses object_property_get_uint() to replace the direct memory
>> access to make [get|set]_addr with the same mechanism.
>> 
>> Signed-off-by: Wei Yang 
>> ---
>>  hw/mem/pc-dimm.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
>> index 0c9b9e8292..c0658b9b88 100644
>> --- a/hw/mem/pc-dimm.c
>> +++ b/hw/mem/pc-dimm.c
>> @@ -204,9 +204,7 @@ static MemoryRegion 
>> *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>>  
>>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>>  {
>> -const PCDIMMDevice *dimm = PC_DIMM(md);
>> -
>> -return dimm->addr;
>> +return object_property_get_uint(OBJECT(md), PC_DIMM_ADDR_PROP, NULL);
>it's not good to ignore errors
>
>s/NULL/error_abort/
>
>so we would notice if error ever happened.

You mean something like this?

Error *err;
uint64_t addr;

addr = object_property_get_uint(OBJECT(md), PC_DIMM_ADDR_PROP, );

if (err) {
warn_report_err();
return 0;
}

return addr;

>
>>  }
>>  
>>  static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
>

-- 
Wei Yang
Help you, Help me

Re: [Qemu-devel] [PATCH] pc-dimm: use same mechanism for [get|set]_addr

2019-02-06 Thread Igor Mammedov 
On Fri,  1 Feb 2019 09:08:37 +0800
Wei Yang  wrote:

> [get|set]_addr are two counterpart to access PCDIMMDevice.addr.
> 
> Since we have already set up a property PC_DIMM_ADDR_PROP for this
> field and use this mechanism in set_addr, it would be more proper to use
> the same mechanism in set_addr.
> 
> This patch uses object_property_get_uint() to replace the direct memory
> access to make [get|set]_addr with the same mechanism.
> 
> Signed-off-by: Wei Yang 
> ---
>  hw/mem/pc-dimm.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 0c9b9e8292..c0658b9b88 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -204,9 +204,7 @@ static MemoryRegion 
> *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error **errp)
>  
>  static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
>  {
> -const PCDIMMDevice *dimm = PC_DIMM(md);
> -
> -return dimm->addr;
> +return object_property_get_uint(OBJECT(md), PC_DIMM_ADDR_PROP, NULL);
it's not good to ignore errors

s/NULL/error_abort/

so we would notice if error ever happened.

>  }
>  
>  static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,

[Qemu-devel] [PATCH] pc-dimm: use same mechanism for [get|set]_addr

2019-01-31 Thread Wei Yang 
[get|set]_addr are two counterpart to access PCDIMMDevice.addr.

Since we have already set up a property PC_DIMM_ADDR_PROP for this
field and use this mechanism in set_addr, it would be more proper to use
the same mechanism in set_addr.

This patch uses object_property_get_uint() to replace the direct memory
access to make [get|set]_addr with the same mechanism.

Signed-off-by: Wei Yang 
---
 hw/mem/pc-dimm.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
index 0c9b9e8292..c0658b9b88 100644
--- a/hw/mem/pc-dimm.c
+++ b/hw/mem/pc-dimm.c
@@ -204,9 +204,7 @@ static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice 
*dimm, Error **errp)
 
 static uint64_t pc_dimm_md_get_addr(const MemoryDeviceState *md)
 {
-const PCDIMMDevice *dimm = PC_DIMM(md);
-
-return dimm->addr;
+return object_property_get_uint(OBJECT(md), PC_DIMM_ADDR_PROP, NULL);
 }
 
 static void pc_dimm_md_set_addr(MemoryDeviceState *md, uint64_t addr,
-- 
2.19.1