From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>,
"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
lkml <linux-kernel@vger.kernel.org>,
Juergen Gross <jgross@suse.com>
Subject: Re: [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing
Date: Wed, 22 Sep 2021 09:56:53 -0400 [thread overview]
Message-ID: <ccf710f1-3c2f-880c-fa89-64fabe852527@oracle.com> (raw)
In-Reply-To: <1374b8da-1076-63fb-bc54-5be9f1ae94d4@suse.com>
On 9/22/21 9:39 AM, Jan Beulich wrote:
> On 22.09.2021 15:29, Boris Ostrovsky wrote:
>> On 9/22/21 6:17 AM, Jan Beulich wrote:
>>> @@ -817,7 +818,7 @@ static long privcmd_ioctl_mmap_resource(
>>> unsigned int i;
>>>
>>> for (i = 0; i < num; i++) {
>>> - rc = pfns[i];
>>> + rc = errs[i];
>>> if (rc < 0)
>>> break;
>>
>> Can the assignment be moved inside the 'if' statement?
> I wouldn't mind, albeit it's not the purpose of this change. Plus
> generally, when I do such elsewhere, I'm frequently told to better
> leave things as separate statements. IOW I'm a little surprised by
> the request.
Sure, can be done as a separate patch.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>
>> I am also not sure I understand why we need error array at all. Don't we always look at the first error only? In fact, AFAICS this is the only place where we look at the value.
> Well, to look at the first error we need to scan the array to find
> one. Indeed we bail from here in once we've found a slot which has
> failed.
>
> I guess what you're trying to say is that there's room for
> improvement. In which case I might agree, but would want to point
> out that doing so would mean removing flexibility from the
> underlying function(s) (which may or may not be fine depending on
> what existing and future requirements there are).
We haven't needed this for a while and IMO existing code, with overloading the meaning of the pfn array, is rather confusing, unnecessarily complicated and error-prone (thus your patch).
> And that would
> be for another day, if at all.
True.
next prev parent reply other threads:[~2021-09-22 13:57 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-22 10:14 [PATCH 0/3] xen/privcmd: misc corrections Jan Beulich
2021-09-22 10:16 ` [PATCH RFC 1/3] xen/privcmd: replace kcalloc() by kvcalloc() when allocating empty pages Jan Beulich
2021-09-23 12:30 ` Juergen Gross
2021-09-22 10:17 ` [PATCH 2/3] xen/privcmd: fix error handling in mmap-resource processing Jan Beulich
2021-09-22 13:29 ` Boris Ostrovsky
2021-09-22 13:39 ` Jan Beulich
2021-09-22 13:56 ` Boris Ostrovsky [this message]
2021-09-22 10:18 ` [PATCH 3/3] xen/privcmd: drop "pages" parameter from xen_remap_pfn() Jan Beulich
2021-09-22 14:02 ` Boris Ostrovsky
2021-10-05 6:39 ` [PATCH 0/3] xen/privcmd: misc corrections Juergen Gross
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=ccf710f1-3c2f-880c-fa89-64fabe852527@oracle.com \
--to=boris.ostrovsky@oracle.com \
--cc=jbeulich@suse.com \
--cc=jgross@suse.com \
--cc=linux-kernel@vger.kernel.org \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.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).