[Stgt-devel] [Patch 1/3] Rework of mnemonic ASC/ASCQ values
Mark Harvey
markh794
Wed Jun 27 23:58:43 CEST 2007
On 6/27/07, FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
> From: Mark Harvey <markh794 at gmail.com>
> Subject: [Stgt-devel] [Patch 1/3] Rework of mnemonic ASC/ASCQ values
> Date: Sat, 23 Jun 2007 18:11:52 +1000
>
> > >From d163aa03f036609ac5343c2d8983aba38ecec679 Mon Sep 17 00:00:00 2001
> > From: Mark Harvey <markh794 at gmail.com>
> > Date: Fri, 22 Jun 2007 18:20:54 +1000
> > Subject: Use nmemonic codes for SENSE codes instead of numeric value.
> >
> > Using nmemonic representation for ASC/ASCQ as a 16bit value.
> > - Values defined in sense_codes.h
> > - Updated sense_data_build() to accept a single 16bit
> > combined asc/ascq value.
> >
> > Signed-off-by: Mark Harvey <markh794 at gmail.com>
> > ---
> > usr/bs_sync.c | 3 +-
> > usr/ibmvio/ibmvio.c | 15 ++++---
> > usr/mmc.c | 7 ++-
> > usr/osd.c | 5 +-
> > usr/sbc.c | 33 +++++++++-------
> > usr/scsi.c | 13 +++---
> > usr/sense_codes.h | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > usr/spc.c | 19 +++++----
> > usr/spt.c | 5 +-
> > usr/tgtd.h | 3 +-
> > 10 files changed, 161 insertions(+), 45 deletions(-)
> > create mode 100644 usr/sense_codes.h
> >
> > diff --git a/usr/bs_sync.c b/usr/bs_sync.c
> > index 6789b5b..4986c59 100644
> > --- a/usr/bs_sync.c
> > +++ b/usr/bs_sync.c
> > @@ -37,6 +37,7 @@
> > #include "util.h"
> > #include "tgtd.h"
> > #include "scsi.h"
> > +#include "sense_codes.h"
> >
> > #define NR_WORKER_THREADS 4
> >
> > @@ -161,7 +162,7 @@ static void *bs_sync_worker_fn(void *arg)
> > eprintf("io error %p %x %d %d %" PRIu64 ", %m\n",
> > cmd, cmd->scb[0], ret, cmd->len, cmd->offset);
> > cmd->result = SAM_STAT_CHECK_CONDITION;
> > - sense_data_build(cmd, MEDIUM_ERROR, 0x11, 0x0);
> > + sense_data_build(cmd, MEDIUM_ERROR, ASC_READ_ERROR);
> > }
> >
> > pthread_mutex_lock(&info->finished_lock);
> > diff --git a/usr/ibmvio/ibmvio.c b/usr/ibmvio/ibmvio.c
> > index 5a2f8fe..8cfc8d6 100644
> > --- a/usr/ibmvio/ibmvio.c
> > +++ b/usr/ibmvio/ibmvio.c
> > @@ -44,6 +44,7 @@
> > #include "target.h"
> > #include "driver.h"
> > #include "spc.h"
> > +#include "sense_codes.h"
> >
> > #define GETTARGET(x) ((int)((((uint64_t)(x)) >> 56) & 0x003f))
> > #define GETBUS(x) ((int)((((uint64_t)(x)) >> 53) & 0x0007))
> > @@ -140,7 +141,8 @@ static int ibmvio_inquiry(int host_no, struct scsi_cmd *cmd)
> > {
> > int ret = SAM_STAT_CHECK_CONDITION;
> > uint8_t *data, *scb = cmd->scb;
> > - unsigned char key = ILLEGAL_REQUEST, asc = 0x24;
> > + unsigned char key = ILLEGAL_REQUEST;
> > + uint16_t asc = ASC_INVALID_FIELD_IN_CDB;
> >
> > if (((scb[1] & 0x3) == 0x3) || (!(scb[1] & 0x3) && scb[2]))
> > goto sense;
> > @@ -148,7 +150,7 @@ static int ibmvio_inquiry(int host_no, struct scsi_cmd *cmd)
> > data = valloc(pagesize);
> > if (!data) {
> > key = HARDWARE_ERROR;
> > - asc = 0;
> > + asc = ASC_INTERNAL_TGT_FAILURE;
>
> Just ASC_INTERNAL_FAILURE would look better?
Took the Values & names from the t10 SPC doco
This was INTERNAL TARGET FAILURE, however I shortened it from TARGET to TGT
I can shorten this again to ASC_INTERNAL_FAILURE if you like..
>
>
> > diff --git a/usr/scsi.c b/usr/scsi.c
> > index 36722e1..5ddc94c 100644
> > --- a/usr/scsi.c
> > +++ b/usr/scsi.c
> > @@ -38,27 +38,28 @@
> > #include "scsi.h"
> > #include "spc.h"
> >
> > -void sense_data_build(struct scsi_cmd *cmd, uint8_t key, uint8_t asc, uint8_t asq)
> > +void sense_data_build(struct scsi_cmd *cmd, uint8_t key, uint16_t asc)
> > {
> > + uint16_t *sense_code;
> > +
> > if (cmd->dev->attrs.sense_format) {
> > /* descriptor format */
> > + sense_code = (uint16_t *)&cmd->sense_buffer[2];
> >
> > cmd->sense_buffer[0] = 0x72; /* current, not deferred */
> > cmd->sense_buffer[1] = key;
> > - cmd->sense_buffer[2] = asc;
> > - cmd->sense_buffer[3] = asq;
> > cmd->sense_len = 8;
> > } else {
> > /* fixed format */
> > + sense_code = (uint16_t *)&cmd->sense_buffer[12];
> >
> > int len = 0xa;
> > - cmd->sense_buffer[0] = 0x70;
> > + cmd->sense_buffer[0] = 0x70; /* current, not deferred */
> > cmd->sense_buffer[2] = key;
> > cmd->sense_buffer[7] = len;
> > - cmd->sense_buffer[12] = asc;
> > - cmd->sense_buffer[13] = asq;
> > cmd->sense_len = len + 8;
> > }
> > + *sense_code = __cpu_to_be16(asc);
> > }
>
> This doesn't work on big-endian boxes?
>
> I prefer more simple code like:
>
> void sense_data_build(struct scsi_cmd *cmd, uint8_t key, uint16_t code)
> {
> uint8_t asc, asq;
>
> asc = code >> 8;
> asq = code & 0xff;
>
> if (cmd->dev->attrs.sense_format) {
> cmd->sense_buffer[0] = 0x72; /* current, not deferred */
> cmd->sense_buffer[1] = key;
> cmd->sense_buffer[2] = asc;
> cmd->sense_buffer[3] = asq;
> cmd->sense_len = 8;
>
No problems at all.
I'll roll this back..
I did have:
cmd->sense_buffer[2] = (code >> 8) & 0xff;
cmd->sense_buffer[3] = code & 0xff;
> > #define TGT_INVALID_DEV_ID ~0ULL
> > diff --git a/usr/sense_codes.h b/usr/sense_codes.h
> > new file mode 100644
> > index 0000000..ea8eebc
> > --- /dev/null
> > +++ b/usr/sense_codes.h
> > @@ -0,0 +1,103 @@
> > +/*
> > + * The SCSI sense key Additional Sense Code / Additional Sense Code Qualifier
> > + *
> > + * Copyright (C) 2007 Mark Harvey markh794 at gmail dot com
>
> How about:
>
> + * Copyright (C) 2007 Mark Harvey <markh794 at gmail.com>
>
> Looks better.
>
> I don't think camouflaging your address is useful since spammers can
> find your address in the git history.
Actually, I'm not even sure I should have a "Copyright" here as the
details have been taken from information at www.t10.org
Should I remove my "Copyright" message altogether or leave it there
and update as per your suggestion ?
Cheers
Mark
More information about the stgt
mailing list