Re: [PATCH v3 02/10] xen/version: Introduce non-truncating deterministically-signed XENVER_* subops

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.08.2023 04:58, Andrew Cooper wrote:
> On 16/08/2023 1:19 am, Stefano Stabellini wrote:
>> On Tue, 15 Aug 2023, Andrew Cooper wrote:
>>> --- a/xen/include/public/version.h
>>> +++ b/xen/include/public/version.h
>>> @@ -19,12 +19,20 @@
>>>  /* arg == NULL; returns major:minor (16:16). */
>>>  #define XENVER_version      0
>>>  
>>> -/* arg == xen_extraversion_t. */
>>> +/*
>>> + * arg == xen_extraversion_t.
>>> + *
>>> + * This API/ABI is broken.  Use XENVER_extraversion2 where possible.
>> Like Jan and Julien I also don't like the word "broken" especially
>> without explanation of why it is broken next to it.
> 
> The word "broken" is an accurate and appropriate word in the English
> language to describe the situation.  I'm sorry you don't like it, but
> unless any of you can articulate why you think it is inappropriate
> phraseology, the complaint is unactionable.

That's simply not true. A wording change is very well possible, and
what you regard as "broken" may not be viewed as such by others. IOW
while "broken" may be "an accurate and appropriate word in the English
language to describe" your perspective of the situation, it may not be
for others. Take the extraversion case: It was clear from the
beginning that it means to limit what to use as XEN_EXTRAVERSION. No
bug, just a limitation. Similarly for the command line: Besides the
full command line (supposedly) being of use for informational purposes
only anyway (and the full one can be taken from the log), no-one could
ever predict at that time that we'd accumulate such a massive amount
of command line options. Again a limitation (and yes, I understand
that the information may disappear from the ring buffer, so "xl dmesg"
after the system was up for a while may not have that data anymore,
yet of course an agent in the system could make the boot messaged
persistent), but not really a bug.

> Specifically ...
> 
>> Instead, I would say:
>>
>> "XENVER_extraversion is deprecated. Please use XENVER_extraversion2."
> 
> ... depreciated is misleading.
> 
> It would be acceptable for a case where we'd introduced a foo2 to add a
> new feature to the interface, but we're being forced to make the new
> interface to fix two bugs and a mis-feature in existing interface.

See above. The existing interfaces are sufficient for the common case.
The extended versions therefore are an enhancement, allowing to call
the original ones deprecated, but not really anything more.

>> If you want to convey the message that the API has problems, then I would
>> say:
>>
>> "XENVER_extraversion might cause truncation. Please use 
>> XENVER_extraversion2."
>>
>> Or even:
>>
>> "XENVER_extraversion has problems. Please use XENVER_extraversion2."
> 
> If "broken" is too nondescript, then "might" is bad too and "has
> problems" is out of the question.
> 
> 
> There is a partial description of the ABI problems in the comment block
> beside the new ops.  I could be persuaded to extend it to be a full list
> of the ABI breakages.
> 
> These header files are a succinct technical reference for proficient
> programmers to interact with Xen.  They are not a tutorial on writing C,
> nor are they appropriate places to sentences of no useful value.
> 
> Through this series, I have done the hard work of updating the
> commonly-used interfaces such that downstreams can stop working around
> real problems caused by real errors in these APIs.

Or, as I view it, working around limitations of the original ABI. Hence
a new feature, not so much a bug fix.

I'm afraid the main reason why this series hasn't made it in yet is your
apparent unwillingness to accept that other people may look at things
differently than you do, resulting in you not being willing to make a
compromise on the chosen wording. While you may view this as others
blocking your work, I don't think it realistically can be called this
way.

Jan