Re: [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 15:43 +0200, Paolo Bonzini wrote:
> Il 23/08/2013 11:23, Alexey Kardashevskiy ha scritto:
> > The existing driver just dropped unsupported requests. This adds error
> > responses to those unhandled requests.
> > 
> > Signed-off-by: Alexey Kardashevskiy <address@hidden>
> > ---
> >  hw/scsi/spapr_vscsi.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> > index cc35b1b..9259d7e 100644
> > --- a/hw/scsi/spapr_vscsi.c
> > +++ b/hw/scsi/spapr_vscsi.c
> > @@ -957,24 +957,24 @@ static int vscsi_handle_mad_req(VSCSIState *s, 
> > vscsi_req *req)
> >          break;
> >      case VIOSRP_ERROR_LOG_TYPE:
> >          fprintf(stderr, "Unsupported ERROR LOG MAD IU\n");
> 
> Please use error_report...

So we have this discussion on IRC and I somewhat disagree..

Yes, this driver should be updated to use error_report. However, it
currently doesn't and I think it should remain consistent. IE. This
patch should use whatever the driver already uses and a separate patch
should convert the whole thing to error_report.

> > -        mad->error_log.common.status = cpu_to_be16(1);
> > -        vscsi_send_iu(s, req, sizeof(mad->error_log), VIOSRP_MAD_FORMAT);
> >          break;
> >      case VIOSRP_ADAPTER_INFO_TYPE:
> >          vscsi_send_adapter_info(s, req);
> > -        break;
> > +        return 1;
> >      case VIOSRP_HOST_CONFIG_TYPE:
> > -        mad->host_config.common.status = cpu_to_be16(1);
> > -        vscsi_send_iu(s, req, sizeof(mad->host_config), VIOSRP_MAD_FORMAT);
> > +        fprintf(stderr, "Unsupported HOST CONFIG TYPE MAD IU\n");
> 
> ... especially in this new fprintf...
> 
> >          break;
> >      case VIOSRP_CAPABILITIES_TYPE:
> >          vscsi_send_capabilities(s, req);
> > -        break;
> > +        return 1;
> >      default:
> >          fprintf(stderr, "VSCSI: Unknown MAD type %02x\n",
> >                  be32_to_cpu(mad->empty_iu.common.type));
> 
> ... and here too, please.
> 
> Thanks,
> 
> Paolo
> 
> >      }
> >  
> > +    mad->empty_iu.common.status = cpu_to_be16(VIOSRP_MAD_NOT_SUPPORTED);
> > +    vscsi_send_iu(s, req, mad->empty_iu.common.length, VIOSRP_MAD_FORMAT);
> > +
> >      return 1;
> >  }
> >  
> >