Re: [PATCH 4/4] xen/version: Introduce non-truncating XENVER_* subops

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 05/01/2023 8:15 am, Jan Beulich wrote:
> On 04.01.2023 19:34, Andrew Cooper wrote:
>> On 04/01/2023 5:04 pm, Jan Beulich wrote:
>>> On 03.01.2023 21:09, Andrew Cooper wrote:
>>>> API/ABI wise, XENVER_build_id could be merged into xenver_varstring_op(), 
>>>> but
>>>> the internal infrastructure is awkward.
>>> I guess build-id also doesn't fit the rest because of not returning strings,
>>> but indeed an array of bytes. You also couldn't use strlen() on the array.
>> The format is unspecified, but it is a base64 encoded ASCII string (not
>> NUL terminated).
> Where are you taking this base64 encoding from? I see the build ID being pure
> binary data, which could easily have an embedded nul.

Oh, so it is.

I'd failed to spot that libxl formats it automatically on behalf of the
caller.

>>>> +    if ( sz > INT32_MAX )
>>>> +        return -E2BIG; /* Compat guests.  2G ought to be plenty. */
>>> While the comment here and in the public header mention compat guests,
>>> the check is uniform. What's the deal?
>> Well its either this, or a (comat ? INT32_MAX : INT64_MAX) check, along
>> with the ifdefary and predicates required to make that compile.
>>
>> But there's not a CPU today which can actually move 2G of data (which is
>> 4G of L1d bandwidth) without suffering the watchdog (especially as we've
>> just read it once for strlen(), so that's 6G of bandwidth), nor do I
>> expect this to change in the forseeable future.
>>
>> There's some boundary (probably far lower) beyond which we can't use the
>> algorithm here.
>>
>> There wants to be some limit, and I don't feel it is necessary to make
>> it variable on the guest type.
> Sure. My question was merely because of the special mentioning of 32-bit /
> compat guests. I'm fine with the universal limit, and I'd also be fine
> with a lower (universal) bound. All I'm after is that the (to me at least)
> confusing comments be adjusted.

How about 16k then?

>> But overall, I'm not seeing a major objection to this change?  In which
>> case I'll go ahead and do the tools/ cleanup too for v2.
> Well, I'm not entirely convinced of the need for the new subops (we could
> as well introduce build time checks to make sure no truncation will occur
> for the existing ones), but I also won't object to their introduction. At
> least for the command line I can see that we will want (need) to support
> more than 1k worth of a string, considering how many options we have
> accumulated.

I've legitimate customer bugs caused by the cmdline limit, and real test
problems caused by the extraversion limit which I'm unwilling to "fix"
by sticking to the current API/ABI.

~Andrew