All of lore.kernel.org
 help / color / mirror / Atom feed
From: Douglas Gilbert <dgilbert@interlog.com>
To: Tomas Winkler <tomas.winkler@intel.com>,
	"James E.J. Bottomley\"" <JBottomley@parallels.com>
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH] scsi_debug: rework resp_report_luns
Date: Tue, 24 Feb 2015 18:42:14 -0500	[thread overview]
Message-ID: <54ED0C56.7040900@interlog.com> (raw)
In-Reply-To: <1424813825-1970-1-git-send-email-tomas.winkler@intel.com>

On 15-02-24 04:37 PM, Tomas Winkler wrote:
> 1. Fix the error check: the alloc length should be > 16
> and not > 4

You are proposing to make a marginally incorrect test
completely incorrect!

The governing rule for almost all "allocation length" fields
in SCSI commands (that return data-in) is:

"An allocation length of zero specifies that no data shall
be transferred. This condition shall not be considered an
error." [in section 4.2.5.6 of spc5r03.pdf and REPORT LUNS
refers to this]

If the REPORT LUNS allocation length is less that 4 then
the caller doesn't even get the response length back so that
has no practical use IMO. If allocation_length=0 and the SCSI
status is GOOD then all that tells you is that REPORT LUNS is
supported, but it has been a mandatory command for 10 years
so that is to be expected.

> 2. Remove duplicated boundary checks which simplify
> the fill-in loop
> 3. Use more of scsi generic API

Something else you might fix is the stack allocation of
2048+8 bytes:
    u8 arr[SDEBUG_RLUN_ARR_SZ];

It would be better to work out how many LUs the
parent target has and use kmalloc() (or friend)
to make a response buffer large enough. And
"large enough" needs to be no larger than the
allocation_length indicates however that requires
extra care in the loop stopping conditions.

Doug Gilbert



  reply	other threads:[~2015-02-24 23:42 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-24 21:37 [PATCH] scsi_debug: rework resp_report_luns Tomas Winkler
2015-02-24 23:42 ` Douglas Gilbert [this message]
2015-02-25  7:54   ` Winkler, Tomas
2015-02-25 16:22     ` Douglas Gilbert
2015-02-25 18:10       ` Winkler, Tomas
2015-02-25 17:01     ` Elliott, Robert (Server Storage)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=54ED0C56.7040900@interlog.com \
    --to=dgilbert@interlog.com \
    --cc=JBottomley@parallels.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=tomas.winkler@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.