public inbox for linux-scsi@vger.kernel.org
 help / color / mirror / Atom feed
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Don Brace <brace77070@gmail.com>
Cc: Joe Handzik <joseph.t.handzik@hp.com>,
	"James E.J. Bottomley" <JBottomley@odin.com>,
	Kevin Barnett <kevin.barnett@pmcs.com>,
	Scott Teel <scott.teel@pmcs.com>, Tomas Henzl <thenzl@redhat.com>,
	iss_storagedev@hp.com, storagedev@pmcs.com,
	linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] scsi: hpsa: fix multiple issues in path_info_show
Date: Sat, 14 Nov 2015 16:24:24 +0100	[thread overview]
Message-ID: <87mvuga9jr.fsf@rasmusvillemoes.dk> (raw)
In-Reply-To: <56313105.3050805@pmcs.com> (Don Brace's message of "Wed, 28 Oct 2015 15:33:09 -0500")

On Wed, Oct 28 2015, Don Brace <brace77070@gmail.com> wrote:

> On 10/27/2015 05:16 PM, Rasmus Villemoes wrote:
>> I'm not familiar with this code, but path_info_show() (added in
>> 8270b86243658 "hpsa: add sysfs entry path_info to show box and
>> bay information") seems to be broken in multiple ways.
>>
[snip]
>>
>> We can fix all of that and get rid of the 400 byte stack buffer by
>> simply writing directly to the given output buffer, which the upper
>> layer guarantees is at least PAGE_SIZE. s[c]nprintf doesn't care where
>> it is writing to, so this doesn't make the spin lock hold time any
>> longer. Using scnprintf ensures that output_len always represents the
>> number of bytes actually written to the buffer, so we'll report the
>> proper amount to the upper layer.
>>
>> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
>>
> Thanks, I added this to my current patch set. This patch will be up
> with you as the author soon.

I see it in mainline now. May I ask why 6 out of 7 scnprintfs were
changed (back) to snprintf? I don't think there's any functional
difference as long as PAGE_SIZE is indeed sufficient, but mixing
snprintf and scnprintf is pretty odd, and there's now a discrepancy
between the commit log and the patch which wasn't in my original - I'd
expect a "[use snprintf because xyz]" note added if the change was
intentional.

Rasmus

  reply	other threads:[~2015-11-14 15:24 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-27 22:16 [PATCH] scsi: hpsa: fix multiple issues in path_info_show Rasmus Villemoes
2015-10-28 20:33 ` Don Brace
2015-11-14 15:24   ` Rasmus Villemoes [this message]
2015-11-18 18:27     ` Don Brace

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=87mvuga9jr.fsf@rasmusvillemoes.dk \
    --to=linux@rasmusvillemoes.dk \
    --cc=JBottomley@odin.com \
    --cc=brace77070@gmail.com \
    --cc=iss_storagedev@hp.com \
    --cc=joseph.t.handzik@hp.com \
    --cc=kevin.barnett@pmcs.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=scott.teel@pmcs.com \
    --cc=storagedev@pmcs.com \
    --cc=thenzl@redhat.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox