[Stgt-devel] [Patch 1/2] Add-new-module-entry-points-for-configuration - Try 5

[Mark Harvey]

On 6/9/07, FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp> wrote:
> From: "Mark Harvey" <markh794 at gmail.com>
> Subject: [Stgt-devel] [Patch 1/2]       Add-new-module-entry-points-for-configuration - Try 5
> Date: Tue, 5 Jun 2007 13:18:04 +1000
>
> > OK, just when I thought I'd found all my coding blunders...
> >
> > A quick 'grep "if\(" identified some of my mistakes and a couple of
> > other that slipped thru.
>
> Applied, thanks a lot. No base64 attachment next time, please.
>
> Some comments for next time.
>
> >  int spc_inquiry(int host_no, struct scsi_cmd *cmd)
> >  {
> > @@ -40,7 +44,7 @@ int spc_inquiry(int host_no, struct scsi_cmd *cmd)
> >       uint8_t *data;
> >       uint8_t *scb = cmd->scb;
> >       unsigned char device_type = cmd->c_target->dev_type_template.type;
> > -     char *product_id = cmd->c_target->dev_type_template.pid;
> > +     struct lu_phy_attr *attributes = cmd->dev->attributes;
> >       unsigned char key = ILLEGAL_REQUEST, asc = 0x24;
> >
> >       if (((scb[1] & 0x3) == 0x3) || (!(scb[1] & 0x3) && scb[2]))
> > @@ -57,22 +61,21 @@ int spc_inquiry(int host_no, struct scsi_cmd *cmd)
> >       dprintf("%x %x\n", scb[1], scb[2]);
> >
> >       if (!(scb[1] & 0x3)) {
> > +             int i;
> >               data[0] = device_type;
> > -             data[2] = 4;
> > +             data[1] = (attributes->is_removable) ? 0x80 : 0;
> > +             data[2] = 5;    /* SPC-3 */
> >               data[3] = 0x42;
> > -             data[4] = 59;
> >               data[7] = 0x02;
> >               memset(data + 8, 0x20, 28);
> > -             strncpy((char *)data + 8, VENDOR_ID, 8);
> > -             strncpy((char *)data + 16, product_id, 16);
> > -             strncpy((char *)data + 32, PRODUCT_REV, 4);
> > -             data[58] = 0x03;
> > -             data[59] = 0x20;
> > -             data[60] = 0x09;
> > -             data[61] = 0x60;
> > -             data[62] = 0x03;
> > -             data[63] = 0x00;
> > -             len = 64;
> > +             strncpy((char *)data + 8, attributes->vendor_ident, 8);
> > +             strncpy((char *)data + 16, attributes->product_ident, 16);
> > +             strncpy((char *)data + 32, attributes->product_rev, 4);
> > +             for (i=0, len=58; i < VERSION_DESCRIPTOR_LEN; i++, len += 2) {
> > +                     data[len] = (attributes->version_desc[i] >> 8) & 0xff;
> > +                     data[len+1] = attributes->version_desc[i] & 0xff;
> > +             }
>
> Should be:
>
> > +             for (i = 0, len = 58; i < VERSION_DESCRIPTOR_LEN; i++, len += 2) {
>
> Anyway, this part looks odd to me. I rewrote this part.
>
>
> > +struct lu_phy_attr {
> > +     char scsi_id[SCSI_ID_LEN];
> > +     char scsi_sn[SCSI_SN_LEN];
> > +
> > +     /* SCSI Inquiry Params */
> > +     char vendor_ident[9];
> > +     char product_ident[17];
>
> Renamed vendor_id and product_id.
>
> > +     char product_rev[5];
> > +     uint16_t version_desc[VERSION_DESCRIPTOR_LEN];
> > +
> > +     char is_removable;      /* Removable media */
> > +     char online;            /* Logical Unit online ? */
> > +     char reset;             /* Power-on or reset has occured */
> > +     char sense_format;      /* Descrptor format sense data supported */
> > +};
> > +
> >  struct scsi_lu {
> >       int fd;
> >       uint64_t addr; /* persistent mapped address */
> >       uint64_t size;
> >       uint64_t lun;
> > -     char scsi_id[SCSI_ID_LEN];
> > -     char scsi_sn[SCSI_SN_LEN];
> >       char *path;
> >
> >       /* the list of devices belonging to a target */
> > @@ -48,7 +63,7 @@ struct scsi_lu {
> >       uint64_t reserve_id;
> >
> >       /* TODO: needs a structure for lots of device parameters */
> > -     uint8_t d_sense;
> > +     struct lu_phy_attr *attributes;
>
> Why do we use a struct lu_phy_attr pointer? Why just can't we do:
>
> struct scsi_lu {
> ...
>         struct lu_phy_attr attrs;
> };
>
> 'attributes' is too long for me. renamed 'attrs'.
>

I used a pointer and malloc(sizeof(struct lu_phy_attrs)) thinking this
will limit the size of the scsi_lu structure.

I can in-line it with scsi_lu if you like..


BTW: I am having problem accessing the git repositry via http..
(git:// blocked from work's firewall)

$ git-pull http://www.kernel.org/pub/scm/linux/kernel/git/tomo/tgt.git master
Fetching refs/heads/master from
http://www.kernel.org/pub/scm/linux/kernel/git/tomo/tgt.git using http
Getting alternates list for
http://www.kernel.org/pub/scm/linux/kernel/git/tomo/tgt.git
Getting pack list for
http://www.kernel.org/pub/scm/linux/kernel/git/tomo/tgt.git
Getting index for pack 347d7aca3fccc4218e6acf919119f6bdabc921a6
Getting index for pack acebc448c87d220eb76e7fa8a8caa8fd04fd1506
Getting pack acebc448c87d220eb76e7fa8a8caa8fd04fd1506
 which contains 468fbf386c3f7df6217353eaba33630cf293f180
walk 468fbf386c3f7df6217353eaba33630cf293f180
[snip]


$ git log
commit 468fbf386c3f7df6217353eaba33630cf293f180
Author: FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp>
Date:   Sat Apr 14 10:03:55 2007 +0900

    fix pread/pwrite large offset bugs

    Signed-off-by: FUJITA Tomonori <fujita.tomonori at lab.ntt.co.jp>

[snip]


Cheers
Mark