| Message ID | 1377249797-12623-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
On 23.08.2013, at 10:23, Alexey Kardashevskiy wrote: > The existing driver just dropped unsupported requests. This adds error > responses to those unhandled requests. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > 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"); These should probably be error_report() calls. > - mad->error_log.common.status = cpu_to_be16(1); > - vscsi_send_iu(s, req, sizeof(mad->error_log), VIOSRP_MAD_FORMAT); This changes the reply from sizeof(mad->error_log) to mad->empty_io.common.length. Is this correct? > break; > case VIOSRP_ADAPTER_INFO_TYPE: > vscsi_send_adapter_info(s, req); > - break; > + return 1; Please turn this into a boolean variable that tells the common code below that we've successfully handled the request. > 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"); > 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)); > } > then you can just put the handling below in an if (!request_handled) { } block which makes the code a lot easier to follow. Multiple exit points of a function are a pretty regular source of breakage, because you're very likely to miss one. > + 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); Doesn't the length have to be endianness swapped too? Alex > + > return 1; > } > > -- > 1.8.4.rc4 >
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 <aik@ozlabs.ru> > --- > 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... > - 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; > } > >
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 <aik@ozlabs.ru> > > --- > > 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; > > } > > > >
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"); - 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"); 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)); } + 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; }
The existing driver just dropped unsupported requests. This adds error responses to those unhandled requests. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/scsi/spapr_vscsi.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)