| Message ID | 1445501858-18790-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 22.10.2015 um 10:17 schrieb Fam Zheng: > Previously we return -EIO blindly when anything goes wrong. Add a helper > function to parse sense fields and try to make the return code more > meaningful. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 55 insertions(+), 1 deletion(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 93f1ee4..f3e20ae 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -84,6 +84,7 @@ typedef struct IscsiTask { > IscsiLun *iscsilun; > QEMUTimer retry_timer; > bool force_next_flush; > + int err_code; > } IscsiTask; > > typedef struct IscsiAIOCB { > @@ -182,6 +183,58 @@ static inline unsigned exp_random(double mean) > #define QEMU_SCSI_STATUS_TIMEOUT SCSI_STATUS_TIMEOUT > #endif > > +static int iscsi_translate_sense(struct scsi_sense *sense) > +{ > + int ret = 0; > + > + switch (sense->key) { > + case SCSI_SENSE_NO_SENSE: > + return 0; > + break; > + case SCSI_SENSE_NOT_READY: > + return -EBUSY; > + break; > + case SCSI_SENSE_DATA_PROTECTION: > + return -EACCES; > + break; > + case SCSI_SENSE_COMMAND_ABORTED: > + return -ECANCELED; > + break; > + case SCSI_SENSE_ILLEGAL_REQUEST: > + /* Parse ASCQ */ > + break; > + default: > + return -EIO; > + break; > + } > + switch (sense->ascq) { > + case SCSI_SENSE_ASCQ_PARAMETER_LIST_LENGTH_ERROR: > + case SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE: > + case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB: > + case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_PARAMETER_LIST: > + ret = -EINVAL; > + break; > + case SCSI_SENSE_ASCQ_LBA_OUT_OF_RANGE: > + ret = -ERANGE; > + break; > + case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED: > + ret = -ENOTSUP; > + break; > + case SCSI_SENSE_ASCQ_WRITE_PROTECTED: > + ret = -EACCES; > + break; > + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT: > + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_CLOSED: > + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_OPEN: > + ret = -ENOMEDIUM; > + break; > + default: > + ret = -EIO; > + break; > + } > + return ret; > +} > + > static void > iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, > void *command_data, void *opaque) > @@ -226,6 +279,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, > return; > } > } > + iTask->err_code = iscsi_translate_sense(&task->sense); > error_report("iSCSI Failure: %s", iscsi_get_error(iscsi)); > } else { > iTask->iscsilun->force_next_flush |= iTask->force_next_flush; > @@ -455,7 +509,7 @@ retry: > } > > if (iTask.status != SCSI_STATUS_GOOD) { > - return -EIO; > + return iTask.err_code; > } why do you only use that translated error code for writev? Other calls could benefit from it as well. Peter
On 22/10/2015 10:31, Peter Lieven wrote: > > + switch (sense->key) { > + case SCSI_SENSE_NO_SENSE: > + return 0; > + break; > + case SCSI_SENSE_NOT_READY: > + return -EBUSY; > + break; > + case SCSI_SENSE_DATA_PROTECTION: > + return -EACCES; Probably EPERM, not EACCES. > + break; > + case SCSI_SENSE_COMMAND_ABORTED: > + return -ECANCELED; > + break; > + case SCSI_SENSE_ILLEGAL_REQUEST: > + /* Parse ASCQ */ > + break; > + default: > + return -EIO; > + break; > + } > + switch (sense->ascq) { > + case SCSI_SENSE_ASCQ_PARAMETER_LIST_LENGTH_ERROR: > + case SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE: > + case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB: > + case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_PARAMETER_LIST: > + ret = -EINVAL; > + break; > + case SCSI_SENSE_ASCQ_LBA_OUT_OF_RANGE: > + ret = -ERANGE; > + break; > + case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED: > + ret = -ENOTSUP; > + break; > + case SCSI_SENSE_ASCQ_WRITE_PROTECTED: > + ret = -EACCES; Same here. Paolo > + break; > + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT: > + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_CLOSED: > + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_OPEN: > + ret = -ENOMEDIUM; > + break; > + default: > + ret = -EIO; > + break; > + }
Am 22.10.2015 um 10:17 hat Fam Zheng geschrieben: > Previously we return -EIO blindly when anything goes wrong. Add a helper > function to parse sense fields and try to make the return code more > meaningful. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 55 insertions(+), 1 deletion(-) > > diff --git a/block/iscsi.c b/block/iscsi.c > index 93f1ee4..f3e20ae 100644 > --- a/block/iscsi.c > +++ b/block/iscsi.c > @@ -84,6 +84,7 @@ typedef struct IscsiTask { > IscsiLun *iscsilun; > QEMUTimer retry_timer; > bool force_next_flush; > + int err_code; > } IscsiTask; > > typedef struct IscsiAIOCB { > @@ -182,6 +183,58 @@ static inline unsigned exp_random(double mean) > #define QEMU_SCSI_STATUS_TIMEOUT SCSI_STATUS_TIMEOUT > #endif > > +static int iscsi_translate_sense(struct scsi_sense *sense) > +{ > + int ret = 0; > + > + switch (sense->key) { > + case SCSI_SENSE_NO_SENSE: > + return 0; > + break; > + case SCSI_SENSE_NOT_READY: > + return -EBUSY; > + break; > + case SCSI_SENSE_DATA_PROTECTION: > + return -EACCES; > + break; > + case SCSI_SENSE_COMMAND_ABORTED: > + return -ECANCELED; > + break; > + case SCSI_SENSE_ILLEGAL_REQUEST: > + /* Parse ASCQ */ > + break; > + default: > + return -EIO; > + break; > + } break after return is dead code. Kevin
Am 22.10.2015 um 10:45 schrieb Paolo Bonzini: > > On 22/10/2015 10:31, Peter Lieven wrote: >> + switch (sense->key) { >> + case SCSI_SENSE_NO_SENSE: >> + return 0; >> + break; Isn't it dangerous to return 0 (no error) if the status is != SCSI_STATUS_GOOD? Peter
On Thu, 10/22 10:45, Paolo Bonzini wrote: > > > On 22/10/2015 10:31, Peter Lieven wrote: > > > > + switch (sense->key) { > > + case SCSI_SENSE_NO_SENSE: > > + return 0; > > + break; > > + case SCSI_SENSE_NOT_READY: > > + return -EBUSY; > > + break; > > + case SCSI_SENSE_DATA_PROTECTION: > > + return -EACCES; > > Probably EPERM, not EACCES. The comment of bdrv_write says -EACCES for read-only device, shoudn't this be the same? Fam
On 22/10/2015 11:51, Fam Zheng wrote: > > > > > > + switch (sense->key) { > > > + case SCSI_SENSE_NO_SENSE: > > > + return 0; > > > + break; > > > + case SCSI_SENSE_NOT_READY: > > > + return -EBUSY; > > > + break; > > > + case SCSI_SENSE_DATA_PROTECTION: > > > + return -EACCES; > > > > Probably EPERM, not EACCES. > > The comment of bdrv_write says -EACCES for read-only device, shoudn't this be > the same? Oh, then that's fine. I was looking at the write(2) man page.
diff --git a/block/iscsi.c b/block/iscsi.c index 93f1ee4..f3e20ae 100644 --- a/block/iscsi.c +++ b/block/iscsi.c @@ -84,6 +84,7 @@ typedef struct IscsiTask { IscsiLun *iscsilun; QEMUTimer retry_timer; bool force_next_flush; + int err_code; } IscsiTask; typedef struct IscsiAIOCB { @@ -182,6 +183,58 @@ static inline unsigned exp_random(double mean) #define QEMU_SCSI_STATUS_TIMEOUT SCSI_STATUS_TIMEOUT #endif +static int iscsi_translate_sense(struct scsi_sense *sense) +{ + int ret = 0; + + switch (sense->key) { + case SCSI_SENSE_NO_SENSE: + return 0; + break; + case SCSI_SENSE_NOT_READY: + return -EBUSY; + break; + case SCSI_SENSE_DATA_PROTECTION: + return -EACCES; + break; + case SCSI_SENSE_COMMAND_ABORTED: + return -ECANCELED; + break; + case SCSI_SENSE_ILLEGAL_REQUEST: + /* Parse ASCQ */ + break; + default: + return -EIO; + break; + } + switch (sense->ascq) { + case SCSI_SENSE_ASCQ_PARAMETER_LIST_LENGTH_ERROR: + case SCSI_SENSE_ASCQ_INVALID_OPERATION_CODE: + case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_CDB: + case SCSI_SENSE_ASCQ_INVALID_FIELD_IN_PARAMETER_LIST: + ret = -EINVAL; + break; + case SCSI_SENSE_ASCQ_LBA_OUT_OF_RANGE: + ret = -ERANGE; + break; + case SCSI_SENSE_ASCQ_LOGICAL_UNIT_NOT_SUPPORTED: + ret = -ENOTSUP; + break; + case SCSI_SENSE_ASCQ_WRITE_PROTECTED: + ret = -EACCES; + break; + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT: + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_CLOSED: + case SCSI_SENSE_ASCQ_MEDIUM_NOT_PRESENT_TRAY_OPEN: + ret = -ENOMEDIUM; + break; + default: + ret = -EIO; + break; + } + return ret; +} + static void iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, void *command_data, void *opaque) @@ -226,6 +279,7 @@ iscsi_co_generic_cb(struct iscsi_context *iscsi, int status, return; } } + iTask->err_code = iscsi_translate_sense(&task->sense); error_report("iSCSI Failure: %s", iscsi_get_error(iscsi)); } else { iTask->iscsilun->force_next_flush |= iTask->force_next_flush; @@ -455,7 +509,7 @@ retry: } if (iTask.status != SCSI_STATUS_GOOD) { - return -EIO; + return iTask.err_code; } iscsi_allocationmap_set(iscsilun, sector_num, nb_sectors);
Previously we return -EIO blindly when anything goes wrong. Add a helper function to parse sense fields and try to make the return code more meaningful. Signed-off-by: Fam Zheng <famz@redhat.com> --- block/iscsi.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-)