[stgt] Seems to be a bug or two in smc.c

[Mark Harvey]

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