[stgt] Seems to be a bug or two in smc.c
Mark Harvey
markh794 at gmail.com
Sat Aug 16 00:21:46 CEST 2008
Comments inline.
On Fri, Aug 15, 2008 at 7:45 PM, FUJITA Tomonori
<fujita.tomonori at lab.ntt.co.jp> wrote:
> Added CC to Mark Harvey,
>
> On Thu, 14 Aug 2008 16:44:24 -0700
> "Richard Sharpe" <realrichardsharpe at gmail.com> wrote:
>
>> In the process of getting BakBone to see my VTL and work with it
>> properly, I think I found a couple of buglets.
>>
>> BakBone issues an INITIALIZE_ELEMENT_STATUS and seems to give up if
>> told that the command was not implemented.
>>
>> Also, we need to properly fill in the LUN for data transfer devices
>> and protect against being asked for too little data, because we can
>> corrupt things.
>>
>> Attached is a possible patch.
>
> =
> --- /home/rsharpe/open-source-devel/linux-scsi-tgt/usr/smc.c 2008-07-30 10:26:58.000000000 -0700
> +++ smc.c 2008-08-14 16:21:30.000000000 -0700
> @@ -142,7 +142,14 @@
> data[3] = 0; /* Reserved */
> data[4] = (s->asc >> 8) & 0xff; /* Additional Sense Code */
> data[5] = s->asc & 0xff; /* Additional Sense Code Qualifier */
> - /* [6], [7] & [8] reserved */
> + /* [6], [7] & [8] reserved except for data transfer element SMC3 */
> + if (element_type == ELEMENT_DATA_TRANSFER) {
> + /*
> + * Only works if on same bus as the media changer and
> + * lun is <= 7 ...
> + */
> + data[6] = 0x10 | (s->drive_lun & 0x07); /* Add in LUN */
> + }
> data[9] = (s->cart_type & 0xf);
>
> if (s->last_addr) { /* Source address is valid ? */
NAK
SMC-3r11 (dated 28 Feb, 2008) lists byte 6 as obsolete or reserved.
It also does not make sense in a iSCSI environment as the DATA
TRANSFER DEVICES will most likely be exported as a different target
and hence a different bus/lun at the initiator side.
> @@ -225,6 +232,23 @@
> }
>
> /**
> + * smc_initialize_element_status - INITIALIZE ELEMENT STATUS op code
> + *
> + * Some backup libraries seem to require this.
> + *
> + * Support the SCSI op code INITIALIZE_ELEMENT_STATUS
> + * Ref: smc3r10a, 6.2
> + */
> +static int smc_initialize_element_status(int host_no, struct scsi_cmd *cmd)
> +{
> + /*
> + * Should do some error checking here ... the spec says some stuff
> + * about having a reservation
> + */
> + return SAM_STAT_GOOD;
> +}
> +
> +/**
ACK
I would recommend submitting the addition of each SCSI OP code as a
unique stand-alone patch.
Another similar patch for INITIALIZE_ELEMENT_STATUS_WITH_RANGE would also
be 'handy' and can also be implemented as a NO-OP.
Although, both INITIALIZE ELEMENT STATUS and INITIALIZE_ELEMENT STATUS
WITH RANGE are listed as optional OP codes (Reference: SMC-3r11), most
backup software require/expect these commands to be supported.
> * smc_read_element_status - READ ELEMENT STATUS op code
> *
> * Support the SCSI op code READ ELEMENT STATUS
> @@ -241,12 +265,14 @@
> uint8_t dvcid;
> int alloc_len;
> uint16_t count = 0;
> + uint32_t slot_count = 0;
> int first = 0; /* First valid slot location */
> int len = 8;
> int elementSize;
> int ret;
> unsigned char key = ILLEGAL_REQUEST;
> uint16_t asc = ASC_INVALID_FIELD_IN_CDB;
> + struct slot *s;
>
> scb = cmd->scb;
> element_type = scb[1] & 0x0f;
> @@ -258,6 +284,14 @@
> if (scsi_get_in_length(cmd) < alloc_len)
> goto sense;
>
> + /*
> + * To protect against given alloc_len less than we need, we allocate
> + * enough memory to build element descriptors for all slots
> + * and only copy what we were asked.
> + */
> + list_for_each_entry(s, &smc->slots, slot_siblings)
> + slot_count++;
> +
> elementSize = determine_element_sz(dvcid, voltag);
>
> scsi_set_in_resid_by_actual(cmd, 0);
> @@ -269,7 +303,7 @@
> }
> }
>
> - data = zalloc(alloc_len);
> + data = zalloc(slot_count * elementSize + len);
> if (!data) {
> dprintf("Can't allocate enough memory for cmd\n");
> key = HARDWARE_ERROR;
> @@ -287,7 +321,7 @@
> ELEMENT_MEDIUM_TRANSPORT,
> &first, req_start_elem,
> dvcid, voltag);
> - len = count * elementSize;
> + len += count * elementSize;
> count += build_element_descriptors(&data[len], &smc->slots,
> ELEMENT_STORAGE,
> &first, req_start_elem,
ACK
> @@ -748,7 +782,7 @@
> {spc_illegal_op,},
> {spc_illegal_op,},
> {spc_illegal_op,},
> - {spc_illegal_op,},
> + {smc_initialize_element_status,},
>
> {spc_illegal_op,},
> {spc_illegal_op,},
>
Cheers
Mark Harvey
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the stgt
mailing list