Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread Paolo Bonzini 


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.

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread Fam Zheng 
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

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread Peter Lieven 

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

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread Kevin Wolf 
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 
> ---
>  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_TIMEOUTSCSI_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

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread Paolo Bonzini 


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;
> +}

Re: [Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread Peter Lieven 

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 
---
  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_TIMEOUTSCSI_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

[Qemu-devel] [PATCH 1/2] iscsi: Translate scsi sense into error code

2015-10-22 Thread 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 
---
 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_TIMEOUTSCSI_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);
-- 
2.4.3