* [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests
@ 2013-08-23 9:23 Alexey Kardashevskiy
2013-08-25 16:26 ` Alexander Graf
2013-08-26 13:43 ` Paolo Bonzini
0 siblings, 2 replies; 4+ messages in thread
From: Alexey Kardashevskiy @ 2013-08-23 9:23 UTC (permalink / raw)
To: qemu-devel; +Cc: Alexey Kardashevskiy, qemu-ppc, Alexander Graf, Paolo Bonzini
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");
- 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;
}
--
1.8.4.rc4
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests
2013-08-23 9:23 [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests Alexey Kardashevskiy
@ 2013-08-25 16:26 ` Alexander Graf
2013-08-26 13:43 ` Paolo Bonzini
1 sibling, 0 replies; 4+ messages in thread
From: Alexander Graf @ 2013-08-25 16:26 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: qemu-ppc, qemu-devel, Paolo Bonzini
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests
2013-08-23 9:23 [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests Alexey Kardashevskiy
2013-08-25 16:26 ` Alexander Graf
@ 2013-08-26 13:43 ` Paolo Bonzini
2013-08-27 0:50 ` Benjamin Herrenschmidt
1 sibling, 1 reply; 4+ messages in thread
From: Paolo Bonzini @ 2013-08-26 13:43 UTC (permalink / raw)
To: Alexey Kardashevskiy; +Cc: qemu-ppc, qemu-devel, Alexander Graf
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;
> }
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests
2013-08-26 13:43 ` Paolo Bonzini
@ 2013-08-27 0:50 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 4+ messages in thread
From: Benjamin Herrenschmidt @ 2013-08-27 0:50 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: Alexey Kardashevskiy, qemu-ppc, qemu-devel, Alexander Graf
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;
> > }
> >
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-08-27 0:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-08-23 9:23 [Qemu-devel] [PATCH] spapr-vscsi: Report error on unsupported MAD requests Alexey Kardashevskiy
2013-08-25 16:26 ` Alexander Graf
2013-08-26 13:43 ` Paolo Bonzini
2013-08-27 0:50 ` Benjamin Herrenschmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).