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

[Cornelia Huck]

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 think we can assume that the callers here all pass in !NULL. Would
probably make sense to change these anyway because (a) better safe than
sorry, and (b) make sure new code does not copy it.