Re: [Qemu-devel] [PATCH 04/10] scsi: introduce sg_io_sense_from_errno

[Philippe Mathieu-Daudé]

Hi Paolo,

On 08/22/2017 10:18 AM, Paolo Bonzini wrote:
> Move more knowledge of SG_IO out of hw/scsi/scsi-generic.c, for
> reusability.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>   hw/scsi/scsi-generic.c | 40 +++++++---------------------------------
>   include/scsi/utils.h   |  3 +++
>   scsi/utils.c           | 35 +++++++++++++++++++++++++++++++++++
>   3 files changed, 45 insertions(+), 33 deletions(-)
>
> diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
> index 7a8f500934..04c687ee76 100644
> --- a/hw/scsi/scsi-generic.c
> +++ b/hw/scsi/scsi-generic.c
> @@ -81,6 +81,7 @@ static void scsi_free_request(SCSIRequest *req)
>   static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
>   {
>       int status;
> +    SCSISense sense;
>   
>       assert(r->req.aiocb == NULL);
>   
> @@ -88,42 +89,15 @@ static void scsi_command_complete_noio(SCSIGenericReq *r, int ret)
>           scsi_req_cancel_complete(&r->req);
>           goto done;
>       }
> -    if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> -        r->req.sense_len = r->io_header.sb_len_wr;
> -    }
> -
> -    if (ret != 0) {
> -        switch (ret) {
> -        case -EDOM:
> -            status = TASK_SET_FULL;
> -            break;
> -        case -ENOMEM:
> -            status = CHECK_CONDITION;
> -            scsi_req_build_sense(&r->req, SENSE_CODE(TARGET_FAILURE));
> -            break;
> -        default:
> -            status = CHECK_CONDITION;
> -            scsi_req_build_sense(&r->req, SENSE_CODE(IO_ERROR));
> -            break;
> -        }
> -    } else {
> -        if (r->io_header.host_status == SG_ERR_DID_NO_CONNECT ||
> -            r->io_header.host_status == SG_ERR_DID_BUS_BUSY ||
> -            r->io_header.host_status == SG_ERR_DID_TIME_OUT ||
> -            (r->io_header.driver_status & SG_ERR_DRIVER_TIMEOUT)) {
> -            status = BUSY;
> -            BADF("Driver Timeout\n");
> -        } else if (r->io_header.host_status) {
> -            status = CHECK_CONDITION;
> -            scsi_req_build_sense(&r->req, SENSE_CODE(I_T_NEXUS_LOSS));
> -        } else if (r->io_header.status) {
> -            status = r->io_header.status;
> -        } else if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> -            status = CHECK_CONDITION;
> +    status = sg_io_sense_from_errno(-ret, &r->io_header, &sense);
> +    if (status == CHECK_CONDITION) {
> +        if (r->io_header.driver_status & SG_ERR_DRIVER_SENSE) {
> +            r->req.sense_len = r->io_header.sb_len_wr;
>           } else {
> -            status = GOOD;
> +            scsi_req_build_sense(&r->req, sense);
>           }
>       }
> +
>       DPRINTF("Command complete 0x%p tag=0x%x status=%d\n",
>               r, r->req.tag, status);
>   
> diff --git a/include/scsi/utils.h b/include/scsi/utils.h
> index 76c3db98c0..c12b34f2e5 100644
> --- a/include/scsi/utils.h
> +++ b/include/scsi/utils.h
> @@ -113,6 +113,9 @@ int scsi_cdb_length(uint8_t *buf);
>   #define SG_ERR_DID_TIME_OUT    0x03
>   
>   #define SG_ERR_DRIVER_SENSE    0x08
> +
> +int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
> +                           SCSISense *sense);
>   #endif
>   
>   #endif
> diff --git a/scsi/utils.c b/scsi/utils.c
> index 54d6d4486a..ca5b058a73 100644
> --- a/scsi/utils.c
> +++ b/scsi/utils.c
> @@ -412,3 +412,38 @@ const char *scsi_command_name(uint8_t cmd)
>       }
>       return names[cmd];
>   }
> +
> +#ifdef CONFIG_LINUX
> +int sg_io_sense_from_errno(int errno_value, struct sg_io_hdr *io_hdr,
> +                           SCSISense *sense)
> +{
> +    if (errno_value != 0) {
> +        switch (errno_value) {
> +        case EDOM:
> +            return TASK_SET_FULL;
> +        case ENOMEM:
> +            *sense = SENSE_CODE(TARGET_FAILURE);
> +            return CHECK_CONDITION;
> +        default:
> +            *sense = SENSE_CODE(IO_ERROR);
> +            return CHECK_CONDITION;
> +        }
> +    } else {
> +        if (io_hdr->host_status == SG_ERR_DID_NO_CONNECT ||
> +            io_hdr->host_status == SG_ERR_DID_BUS_BUSY ||
> +            io_hdr->host_status == SG_ERR_DID_TIME_OUT ||
> +            (io_hdr->driver_status & SG_ERR_DRIVER_TIMEOUT)) {
> +            return BUSY;
> +        } else if (io_hdr->host_status) {
> +            *sense = SENSE_CODE(I_T_NEXUS_LOSS);
> +            return CHECK_CONDITION;
> +        } else if (io_hdr->status) {
> +            return io_hdr->status;
> +        } else if (io_hdr->driver_status & SG_ERR_DRIVER_SENSE) {
> +            return CHECK_CONDITION;
> +        }

> +    }
I find it easier to read with the return GOOD out of the if/else:

       return GOOD;

Either ways:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +}
> +#endif