On Sun, 13 Apr 2008 12:13:18 +0300
Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> On Sat, Apr 12 2008 at 8:52 +0300, FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote:
> > On Sun, 06 Apr 2008 12:35:04 +0300
> > Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> >
> >> On Fri, Apr 04 2008 at 14:46 +0300, Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> >>> On Thu, Apr 03 2008, Boaz Harrosh wrote:
> >>>> static void req_bio_endio(struct request *rq, struct bio *bio,
> >>>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> >>>> index 6f79d40..2f87c9d 100644
> >>>> --- a/include/linux/blkdev.h
> >>>> +++ b/include/linux/blkdev.h
> >>>> @@ -213,8 +213,15 @@ struct request {
> >>>> /*
> >>>> * when request is used as a packet command carrier
> >>>> */
> >>>> - unsigned int cmd_len;
> >>>> - unsigned char cmd[BLK_MAX_CDB];
> >>>> + unsigned short cmd_len;
> >>>> + unsigned short ext_cdb_len; /* length of ext_cdb buffer */
> >>>> + union {
> >>>> + unsigned char cmd[BLK_MAX_CDB];
> >>>> + unsigned char *ext_cdb;/* an optional extended cdb.
> >>>> + * points to a user buffer that must
> >>>> + * be valid until end of request
> >>>> + */
> >>>> + };
> >>> Why not just something ala
> >>>
> >>> unsigned short cmd_len;
> >>> unsigned char __cmd[BLK_MAX_CDB];
> >>> unsigned char *cmd;
> >>>
> >>> and then have rq_init() do
> >>>
> >>> rq->cmd = rq->__cmd;
> >>>
> >>> and just have a function for setting up a larger ->cmd and adjusting
> >>> ->cmd_len in the process?
> >>>
> >>> Then rq_set_cdb() would be
> >>>
> >>> static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)
> >>> {
> >>> rq->cmd = cdb;
> >>> rq->cmd_len = cdb_len;
> >>> }
> >>>
> >>> and rq_get_cdb() plus rq_get_cdb_len() could just go away.
> >>>
> >> Because this way it is dangerous if large commands are issued to legacy
> >> drivers. In scsi-land we have .cmd_len at host template that will govern if
> >> we are allow to issue larger commands to the driver. In block devices we do
> >> not have such a facility, and the danger is if such commands are issued through
> >> bsg or other means, even by malicious code. What you say is the ideal and it
> >> is what I've done for scsi, but for block devices we can not do that yet.
> >> With the way I did it here, Legacy drivers will see zero length command and
> >> will do the right thing, from what I've seen.
> >
> > What are exactly block devices? ub and ide?
> >
> > bsg are created only for scsi devices (and scsi objects like sas host)
> > now. Are there other means to send commands except for ioctl?
>
> I'm not 100% sure either way, so I would like to be safe. Any way, there
> is the size issue, this way we add *nothing* at all, so it looks preferable.
> The final outcome will be the same both ways.
I think that a clean design is an important issue than the sizeof of
struct request.
> I would like if you reconsider the ugliness issue. I admit that at first I
> personally disliked it, but now that I look at it, I think it is cleaner,
> coding style, this way. Because the union points out the exclusiveness of
> the two systems, the striate way give the notion of two separate systems.
That's a ugly hack for me.
Why do we have two separate systems to represent the command length?
If the command length is smaller than 16 bytes, we use cmd_len. If the
length is larger than 16 bytes, we use varlen_cdb_len?
For me, as Jens proposed, having only cmd_len is the right way.
And 'cdb' name is not appropriate for the block layer, I think.
I agreed that changing the block layer and the scsi midlayer gradually
is a safe option. Shortly, I'll send patches to clean up the hack on
the top of your patchset.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
