| Message ID | 20191130194240.10517-14-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Error handling fixes, may contain 4.2 material | expand |
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>
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.
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; }
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(-)