memory_device_get_free_addr() crashes when
memory_device_check_addable() fails and its @errp argument is null.
Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
checks into MemoryDevice".

The bug can't bite as no caller actually passes null.  Fix it anyway.

Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/mem/memory-device.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index aef148c1d7..4bc9cf0917 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
                                             uint64_t align, uint64_t size,
                                             Error **errp)
 {
+    Error *err = NULL;
     GSList *list = NULL, *item;
     Range as, new = range_empty;
 
@@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
         return 0;
     }
 
-    memory_device_check_addable(ms, size, errp);
-    if (*errp) {
+    memory_device_check_addable(ms, size, &err);
+    if (err) {
+        error_propagate(errp, err);
         return 0;
     }
 
-- 
2.21.0

On 30.11.19 20:42, Markus Armbruster wrote:
> memory_device_get_free_addr() crashes when
> memory_device_check_addable() fails and its @errp argument is null.
> Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
> checks into MemoryDevice".
> 
> The bug can't bite as no caller actually passes null.  Fix it anyway.
> 
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/mem/memory-device.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
> index aef148c1d7..4bc9cf0917 100644
> --- a/hw/mem/memory-device.c
> +++ b/hw/mem/memory-device.c
> @@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>                                              uint64_t align, uint64_t size,
>                                              Error **errp)
>  {
> +    Error *err = NULL;
>      GSList *list = NULL, *item;
>      Range as, new = range_empty;
>  
> @@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>          return 0;
>      }
>  
> -    memory_device_check_addable(ms, size, errp);
> -    if (*errp) {
> +    memory_device_check_addable(ms, size, &err);
> +    if (err) {
> +        error_propagate(errp, err);
>          return 0;
>      }

I remember that some time ago, the best practice was to use "local_err",
what is the latest state of that?

I still disagree that these are BUGs or even latent BUGs. If somebody
things these are BUGs and not cleanups, then we should probably have
proper "Fixes: " tags instead.

Reviewed-by: David Hildenbrand <david@redhat.com>


-- 
Thanks,

David / dhildenb

David Hildenbrand <david@redhat.com> writes:

> On 30.11.19 20:42, Markus Armbruster wrote:
>> memory_device_get_free_addr() crashes when
>> memory_device_check_addable() fails and its @errp argument is null.
>> Messed up in commit 1b6d6af21b "pc-dimm: factor out capacity and slot
>> checks into MemoryDevice".
>> 
>> The bug can't bite as no caller actually passes null.  Fix it anyway.
>> 
>> Cc: David Hildenbrand <david@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/mem/memory-device.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>> 
>> diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
>> index aef148c1d7..4bc9cf0917 100644
>> --- a/hw/mem/memory-device.c
>> +++ b/hw/mem/memory-device.c
>> @@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>                                              uint64_t align, uint64_t size,
>>                                              Error **errp)
>>  {
>> +    Error *err = NULL;
>>      GSList *list = NULL, *item;
>>      Range as, new = range_empty;
>>  
>> @@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
>>          return 0;
>>      }
>>  
>> -    memory_device_check_addable(ms, size, errp);
>> -    if (*errp) {
>> +    memory_device_check_addable(ms, size, &err);
>> +    if (err) {
>> +        error_propagate(errp, err);
>>          return 0;
>>      }
>
> I remember that some time ago, the best practice was to use "local_err",
> what is the latest state of that?

Hundreds of local Error * variables are named @local_err, and hundreds
more are named @err.

For what it's worth, the big comment in error.h uses @err, except in one
place where it needs two of them.

> I still disagree that these are BUGs or even latent BUGs. If somebody
> things these are BUGs and not cleanups, then we should probably have
> proper "Fixes: " tags instead.

Let's continue that discussion in the sub-thread where you first raised
this objection.

> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

On 12/1/19 11:07 PM, Markus Armbruster wrote:

>>>   {
>>> +    Error *err = NULL;

>> I remember that some time ago, the best practice was to use "local_err",
>> what is the latest state of that?
> 
> Hundreds of local Error * variables are named @local_err, and hundreds
> more are named @err.
> 
> For what it's worth, the big comment in error.h uses @err, except in one
> place where it needs two of them.

What's more, if we go through with Vladimir's Coccinelle cleanup to use 
ERRP_AUTO_PROPAGATE, then we don't have either name to worry about (both 
'&local_err' and '&err' are replaced by 'errp').

> 
>> I still disagree that these are BUGs or even latent BUGs. If somebody
>> things these are BUGs and not cleanups, then we should probably have
>> proper "Fixes: " tags instead.
> 
> Let's continue that discussion in the sub-thread where you first raised
> this objection.

One benefit of fixing the style (whether or not you count it as a bug 
fix) is that the Coccinelle script for updating to a new style is more 
likely to apply correctly.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org