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

From: Andrew Cooper <Andrew.Cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: George Dunlap <George.Dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Julien Grall <julien@xen.org>,
	Xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 4/4] xen/version: Introduce non-truncating XENVER_* subops
Date: Thu, 5 Jan 2023 22:28:10 +0000	[thread overview]
Message-ID: <63789ce5-5dff-c981-4127-d1ef3227595e@citrix.com> (raw)
In-Reply-To: <f90111d0-b94e-8127-3b13-fbe3558d53f7@suse.com>

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