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

[Alexander Graf]

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 <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");

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
>