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

[Greg Kurz]

On Fri, 8 Jun 2018 13:28:01 +0200
David Hildenbrand <address@hidden> wrote:

> On 08.06.2018 12:52, Greg Kurz wrote:
> > On Fri, 8 Jun 2018 11:24:51 +0200
> > David Hildenbrand <address@hidden> 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.
> 

I agree that we should probably pass &error_abort in many places,
but passing NULL isn't bad per se. It means any failure in the
callee is unimportant enough that we can simply ignore it.

The error framework provides this possibility and so we should
never dereference errp.