From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
"Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support
Date: Fri, 24 Apr 2020 08:11:20 +0200 [thread overview]
Message-ID: <a0671e13-85ea-aeaf-722c-dcfee8b7e930@suse.com> (raw)
In-Reply-To: <62b51cff-4d6f-690b-371a-e6772ea327ab@citrix.com>
On 23.04.2020 19:35, Andrew Cooper wrote:
> On 21/04/2020 07:02, Jan Beulich wrote:
>> On 20.04.2020 20:05, Andrew Cooper wrote:
>>> On 20/04/2020 15:05, Jan Beulich wrote:
>>>> I'm in particular
>>>> concerned that we may gain a large number of such printk()s over
>>>> time, if we added them in such cases.
>>> The printk() was a bit of an afterthought, but deliberately avoiding the
>>> -EINVAL path was specifically not.
>>>
>>> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
>>> they should see something other than
>>>
>>> (XEN) parameter "pv=no-32" unknown!
>> Why - to this specific build of Xen the parameter is unknown.
>
> Because it is unnecessarily problematic and borderline obnoxious to
> users, as well as occasional Xen developers.
>
> "you've not got the correct CONFIG_$X for that to be meaningful" is
> specifically useful to separate from "I've got no idea".
>
>>> I don't think overloading the return value is a clever move, because
>>> then every parse function has to take care of ensuring that -EOPNOTSUPP
>>> (or ENODEV?) never clobbers -EINVAL.
>> I didn't suggest overloading the return value. Instead I
>> specifically want this to go the -EINVAL path.
>>
>>> We could have a generic helper which looks like:
>>>
>>> static inline void ignored_param(const char *cfg, const char *name,
>>> const char *s, const char *ss)
>>> {
>>> printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
>>> cfg, name, s, (int)(ss - s));
>>> }
>>>
>>> which at least would keep all the users consistent.
>> Further bloating the binary with (almost) useless string literals.
>> I'd specifically like to avoid this.
>
> I don't accept that as a valid argument.
>
> We're talking about literally tens of bytes (which will merge anyway, so
> 0 in practice), and a resulting UI which helps people get out of
> problems rather than penalises them for having gotten into a problem to
> begin with.
How will they merge? The different instances of the format string
above may/should, yes, but the different "CONFIG_xyz" literals the
callers pass won't, for example.
> I will absolutely prioritise a more helpful UI over a handful of bytes.
This "a handful of bytes doesn't matter" attitude has lead to
imo unacceptable growth of various software packages over the
years.
Anyway - I don't want to block this change over this argument,
so I'm willing to ack a patch with the helper introduced (and
preferably the "CONFIG_" part of the cfg arguments moved into
the helper's format string), as long as we plan to then make
consistent use of the helper everywhere. That said, I don't
immediately see what would be passed for "cfg" in some of
parse_iommu_param()'s cases.
Jan
next prev parent reply other threads:[~2020-04-24 6:11 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-17 15:50 [PATCH 0/3] x86/pv: Start to trim 32bit support Andrew Cooper
2020-04-17 15:50 ` [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support Andrew Cooper
2020-04-20 13:47 ` Roger Pau Monné
2020-04-20 17:31 ` Andrew Cooper
2020-04-20 14:05 ` Jan Beulich
2020-04-20 18:05 ` Andrew Cooper
2020-04-21 6:02 ` Jan Beulich
2020-04-23 17:35 ` Andrew Cooper
2020-04-24 5:28 ` Jürgen Groß
2020-04-27 20:02 ` Andrew Cooper
2020-04-24 6:11 ` Jan Beulich [this message]
2020-04-20 14:15 ` Jan Beulich
2020-04-29 13:06 ` [PATCH v2 " Andrew Cooper
2020-04-29 13:55 ` Jan Beulich
2020-04-17 15:50 ` [PATCH 2/3] x86/pv: Short-circuit is_pv_{32, 64}bit_domain() in !CONFIG_PV32 builds Andrew Cooper
2020-04-20 14:09 ` [PATCH 2/3] x86/pv: Short-circuit is_pv_{32,64}bit_domain() " Jan Beulich
2020-04-29 13:13 ` Andrew Cooper
2020-04-29 13:29 ` Jan Beulich
2020-04-29 13:30 ` Andrew Cooper
2020-04-29 13:37 ` Jan Beulich
2020-04-17 15:50 ` [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds Andrew Cooper
2020-04-20 14:12 ` Jan Beulich
2020-04-20 14:39 ` Andrew Cooper
2020-04-20 15:47 ` Jan Beulich
2020-04-20 17:08 ` Andrew Cooper
2020-04-21 6:09 ` Jan Beulich
2020-04-18 13:46 ` [PATCH 0/3] x86/pv: Start to trim 32bit support Wei Liu
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=a0671e13-85ea-aeaf-722c-dcfee8b7e930@suse.com \
--to=jbeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.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).