From: NeilBrown <neilb@suse.de>
To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
linux-kernel@vger.kernel.org
Cc: linux-nfs@vger.kernel.org, kernel-janitors@vger.kernel.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()'
Date: Fri, 27 Mar 2020 08:44:17 +1100 [thread overview]
Message-ID: <87r1xe7pvy.fsf@notabene.neil.brown.name> (raw)
In-Reply-To: <2e2d1293-c978-3f1d-5a1e-dc43dc2ad06b@wanadoo.fr>
[-- Attachment #1: Type: text/plain, Size: 2394 bytes --]
On Thu, Mar 26 2020, Christophe JAILLET wrote:
> Le 25/03/2020 à 23:53, NeilBrown a écrit :
>> Can I suggest something more like this:
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index de3c077733a7..0292f45b70f6 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -115,16 +115,9 @@ int svc_print_xprts(char *buf, int maxlen)
>> buf[0] = '\0';
>>
>> spin_lock(&svc_xprt_class_lock);
>> - list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
>> - int slen;
>> -
>> - sprintf(tmpstr, "%s %d\n", xcl->xcl_name, xcl->xcl_max_payload);
>> - slen = strlen(tmpstr);
>> - if (len + slen > maxlen)
>> - break;
>> - len += slen;
>> - strcat(buf, tmpstr);
>> - }
>> + list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list)
>> + len += scnprintf(buf + len, maxlen - len, "%s %d\n",
>> + xcl->xcl_name, xcl->xcl_max_payload);
>> spin_unlock(&svc_xprt_class_lock);
>>
>> return len;
>>
>> NeilBrown
>
> Hi,
>
> this was what I suggested in the patch:
> ---
> This patch should have no functional change.
> We could go further, use scnprintf and write directly in the
> destination
> buffer. However, this could lead to a truncated last line.
> ---
Sorry - I missed that.
So add
end = strrchr(tmpstr, '\n');
if (end)
end[1] = 0;
else
tmpstr[0] = 0;
or maybe something like
list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) {
int l = snprintf(buf + len, maxlen - len, "%s %d\n",
xcl->xcl_name, xcl->xcl_max_payload);
if (l < maxlen - len)
len += l;
}
buf[len] = 0;
There really is no need to have the secondary buffer, and I think doing
so just complicates the code.
That last version is a change of behaviour in that it will skip over
lines that are too long, rather than aborting on the first one.
I don't know which is preferred.
Thanks,
NeilBrown
>
> And Chuck Lever confirmed that:
> That's exactly what this function is trying to avoid. As part of any
> change in this area, it would be good to replace the current block
> comment before this function with a Doxygen-format comment that
> documents that goal.
>
> So, I will only send a V2 based on comments already received.
>
> CJ
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
next prev parent reply other threads:[~2020-03-26 21:44 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-25 7:04 [PATCH 2/2] SUNRPC: Optimize 'svc_print_xprts()' Christophe JAILLET
2020-03-25 14:52 ` Chuck Lever
[not found] ` <42afbf1f-19e1-a05c-e70c-1d46eaba3a71@wanadoo.fr>
2020-03-25 22:53 ` NeilBrown
[not found] ` <2e2d1293-c978-3f1d-5a1e-dc43dc2ad06b@wanadoo.fr>
2020-03-26 21:44 ` NeilBrown [this message]
2020-03-26 22:29 ` Chuck Lever
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=87r1xe7pvy.fsf@notabene.neil.brown.name \
--to=neilb@suse.de \
--cc=christophe.jaillet@wanadoo.fr \
--cc=kernel-janitors@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
/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;
as well as URLs for NNTP newsgroup(s).