Re: [Qemu-devel] [qemu-s390x] [PATCH v1 2/8] spapr: no need to verify the node

[David Hildenbrand <david@redhat.com>]

On 08.06.2018 12:52, Greg Kurz wrote:
> On Fri, 8 Jun 2018 11:24:51 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>>>>>> +1 for error_abort, even if it takes another line.    
>>>>> +1 for error_abort
>>>>> call shouldn't fail, but if does it won't be silently ignored
>>>>> and introduce undefined behavior.    
>>>>
>>>> Maybe we should fix the others that pass in NULL.
>>>>
>>>> (no, not me :D - I'm already busy with your requested pre_plug handling)  
>>> Add it to wiki page for bite sized tasks?  
>>
>> Done.
>>
>>
> 
> FWIW, I've also added a line to check and possibly fix places where we do
> 'if (*errp)', which would cause QEMU to crash if the caller passes NULL.
> 
> $ git grep 'if (\*errp)'
> hmp.c:    if (*errp) {
> hw/ipmi/isa_ipmi_bt.c:    if (*errp)
> hw/ipmi/isa_ipmi_kcs.c:    if (*errp)
> hw/mem/memory-device.c:    if (*errp) {
> hw/mem/memory-device.c:        if (*errp) {
> hw/ppc/spapr.c:        if (*errp) {
> hw/s390x/event-facility.c:        if (*errp) {
> include/qapi/error.h: *     if (*errp) { // WRONG!
> qga/commands-posix.c:            if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:        if (*errp) {
> target/s390x/cpu_models.c:            if (*errp) {
> target/s390x/cpu_models.c:        if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> target/s390x/cpu_models.c:    if (*errp) {
> tests/test-crypto-tlscredsx509.c:    if (*errp) {
> tests/test-io-channel-tls.c:    if (*errp) {
> 

I think the more important part is actually looking out for people that
use NULL instead of error_abort. This way we won't silently ignore errors.

-- 

Thanks,

David / dhildenb