Quantcast

2nd version of SCSI command READ_POSITION 34h

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

2nd version of SCSI command READ_POSITION 34h

denjolras
Hello.
I think I've found some needed enhancement :

Regarding SCSI reference guide, to get logical position on tape, there's 2 version of SCSI command READ_POSITION 34h
byte[1] of the CDB could be 0 or 6h (SHORT / LONG form)

in ssc_read_position ( file ssc.c )
in the case "TAPE_LOADED", we process only the case where byte[1] == 0 or 1 ( what is 1 ?? )

I suggest to replace the
---------------------------
if ((service_action == 0) || (service_action == 1))
    cmd->dbuf_p->sz = resp_read_position(c_pos->blk_number,
                                         cmd->dbuf_p->data,
                                         sam_stat);
---------------------------
by
---------------------------
if ((service_action == 0) || (service_action == 1))
{
    cmd->dbuf_p->sz = resp_read_position(c_pos->blk_number,
                                         cmd->dbuf_p->data,
                                         sam_stat);

} else if (service_action == 6) {
   cmd->dbuf_p->sz = resp_read_position_long(c_pos->blk_number,
                                             cmd->dbuf_p->data,
                                             sam_stat);

} else {
  mkSenseBuf(ILLEGAL_REQUEST, E_INVALID_FIELD_IN_CDB, sam_stat);
  return SAM_STAT_CHECK_CONDITION;
  break;
}
---------------------------

ps:  resp_read_position_long seems bugged, I have to investigate.

pps: extract of SCSI reference guide :
>
> WARNING
> The short form breaks when there are greater than 2^32 logical objects on medium and may become obsolete in
> future standards.
> WARNING
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

denjolras
code of resp_read_position_long ( in vtllib.c ) should also be changed :

it's still incomplete, but better ( it's now working for my need )


int resp_read_position_long(loff_t pos, uint8_t *buf, uint8_t *sam_stat)
{
        uint64_t partition = 1L;

        MHVTL_DBG(1, "resp_read_position_long: Partition %ld, Position %ld", (long)partition,(long)pos);

        memset(buf, 0, READ_POSITION_LONG_LEN); /* Clear 'array' */

        if ((pos == 0) || (pos == 1))
                buf[0] = 0x80;  /* Begining of Partition */

        /* partition should be zero, as we only support one */
        buf[4] = (partition >> 24);
        buf[5] = (partition >> 16);
        buf[6] = (partition >> 8);
        buf[7] = partition;

        /* we don't know logical field identifier */
        buf[8]  = (pos >> 52);
        buf[9]  = (pos >> 48);
        buf[10] = (pos >> 40);
        buf[11] = (pos >> 32);
        buf[12] = (pos >> 24);
        buf[13] = (pos >> 16);
        buf[14] = (pos >> 8);
        buf[15] =  pos;

        return READ_POSITION_LONG_LEN;
}
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

Mark Harvey
In reply to this post by denjolras
denjolras wrote
Hello.
I think I've found some needed enhancement :
Thanks for looking and sending feedback + sample code :)

Nice.

Comments inline.

denjolras wrote
Regarding SCSI reference guide, to get logical position on tape, there's 2 version of SCSI command READ_POSITION 34h
byte[1] of the CDB could be 0 or 6h (SHORT / LONG form)

in ssc_read_position ( file ssc.c )
in the case "TAPE_LOADED", we process only the case where byte[1] == 0 or 1 ( what is 1 ?? )
I need to look this up again, but I know something was using '1' - details escape me at the moment.

denjolras wrote
I suggest to replace the
Any chance of sending in a 'diff -u' format ?
Better yet, if you can install git..

$ git init
$ git pull http://github.com/
<edit away>
Commit your changes: (Prefer one logical change per commit.)
$ git commit -a --signoff

Send me the patch via git:
$ git format-patch -p --stat --keep-subject -o ~/patch/ HEAD^ (where the last 'HEAD^' indicates just the last patch.. Use HEAD^^ for the last 2 patches.. or "git format-patch -p --stat --keep-subject -o ~/patch/ branch_name" for all patches between here and the 'branch_name' reference.)


---------------------------
if ((service_action == 0) || (service_action == 1))
    cmd->dbuf_p->sz = resp_read_position(c_pos->blk_number,
                                         cmd->dbuf_p->data,
                                         sam_stat);
---------------------------
denjolras wrote
by
---------------------------
if ((service_action == 0) || (service_action == 1))
{
I think use of a 'switch' statement on service_action would look better here. (gcc does allow nested switch statements).

denjolras wrote
} else {
  mkSenseBuf(ILLEGAL_REQUEST, E_INVALID_FIELD_IN_CDB, sam_stat);
  return SAM_STAT_CHECK_CONDITION;
  break;
}
Definitely a bug.. Thanks.
Regards from Australia
Mark Harvey
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

Mark Harvey
In reply to this post by denjolras
Comments inline

If you don't want to go to the effort of git, please advise and I'll merge your suggestions into the code base manually.
Otherwise I'll wait for your git formatted patches.

denjolras wrote
it's still incomplete, but better ( it's now working for my need )
Obviously better that what's there now :)
Thanks

denjolras wrote
int resp_read_position_long(loff_t pos, uint8_t *buf, uint8_t *sam_stat)
{
        uint64_t partition = 1L;

        MHVTL_DBG(1, "resp_read_position_long: Partition %ld, Position %ld", (long)partition,(long)pos);

        memset(buf, 0, READ_POSITION_LONG_LEN); /* Clear 'array' */

        if ((pos == 0) || (pos == 1))
                buf[0] = 0x80;  /* Begining of Partition */

        /* partition should be zero, as we only support one */
        buf[4] = (partition >> 24);
        buf[5] = (partition >> 16);
        buf[6] = (partition >> 8);
        buf[7] = partition;
Can you use 'put_unaligned_be32() instead..
e.g.
put_unaligned_be32(partition, &buf[4]);


denjolras wrote
        /* we don't know logical field identifier */
        buf[8]  = (pos >> 52);
        buf[9]  = (pos >> 48);
        buf[10] = (pos >> 40);
        buf[11] = (pos >> 32);
        buf[12] = (pos >> 24);
        buf[13] = (pos >> 16);
        buf[14] = (pos >> 8);
        buf[15] =  pos;

        return READ_POSITION_LONG_LEN;
}
Can you use 'put_unaligned_be64()' instead..
e.g.
put_unaligned_be64(pos, &buf[8]);
Regards from Australia
Mark Harvey
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

Mark Harvey
In reply to this post by Mark Harvey
Reference ssc4r01e:
Table 63:
00h
SHORT FORM -- BLOCK ID
Device server shall return 20 bytes of data with the FIRST LOGICAL OBJECT LOCATION and LAST LOGICAL OBJECT LOCATION fields as logical object identifier values (see 4.2.8.2), relative to a partition.
Mandatory

01h
SHORT FORM -- VENDOR SPECIFIC
Device server shall return 20 bytes of data with the FIRST LOGICAL OBJECT LOCATION and LAST LOGICAL OBJECT LOCATION fields as vendor-specific values.

06h
LONG FORM
Device server shall return 32 bytes of data.

service actions 00 & 06 are listed as 'Mandatory' while 01h is 'Optional'

This definitely needs fixing.
Regards from Australia
Mark Harvey
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

Mark Harvey
In reply to this post by Mark Harvey
Just stating that the code that should be converted to the 'put_unaligned_beXX()' is my old code which needs to be cleaned up.

I'm sure this is not the only place with which do not use the helper functions..
Regards from Australia
Mark Harvey
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

denjolras
In reply to this post by denjolras
ps: I've also a interrogation about this line in resp_read_position_long:
  uint64_t partition = 1L;  

doesn't the partition number start at 0 ?
If yes, the "unique" partition should have number "0", isn't it ?

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: 2nd version of SCSI command READ_POSITION 34h

Mark Harvey
denjolras wrote
doesn't the partition number start at 0 ?
If yes, the "unique" partition should have number "0", isn't it ?
Again, you are correct..

I'll fix this ASAP.
Regards from Australia
Mark Harvey
Loading...