Re: [PATCH 2/4] xen/version: Drop compat/kernel.c

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2023 20:15, Andrew Cooper wrote:
> On 04/01/2023 4:29 pm, Jan Beulich wrote:
>> On 03.01.2023 21:09, Andrew Cooper wrote:
>>> --- a/xen/common/compat/kernel.c
>>> +++ /dev/null
>>> @@ -1,53 +0,0 @@
>>> -/******************************************************************************
>>> - * kernel.c
>>> - */
>>> -
>>> -EMIT_FILE;
>>> -
>>> -#include <xen/init.h>
>>> -#include <xen/lib.h>
>>> -#include <xen/errno.h>
>>> -#include <xen/version.h>
>>> -#include <xen/sched.h>
>>> -#include <xen/guest_access.h>
>>> -#include <asm/current.h>
>>> -#include <compat/xen.h>
>>> -#include <compat/version.h>
>>> -
>>> -extern xen_commandline_t saved_cmdline;
>>> -
>>> -#define xen_extraversion compat_extraversion
>>> -#define xen_extraversion_t compat_extraversion_t
>>> -
>>> -#define xen_compile_info compat_compile_info
>>> -#define xen_compile_info_t compat_compile_info_t
>>> -
>>> -CHECK_TYPE(capabilities_info);
>> This and ...
>>
>>> -#define xen_platform_parameters compat_platform_parameters
>>> -#define xen_platform_parameters_t compat_platform_parameters_t
>>> -#undef HYPERVISOR_VIRT_START
>>> -#define HYPERVISOR_VIRT_START HYPERVISOR_COMPAT_VIRT_START(current->domain)
>>> -
>>> -#define xen_changeset_info compat_changeset_info
>>> -#define xen_changeset_info_t compat_changeset_info_t
>>> -
>>> -#define xen_feature_info compat_feature_info
>>> -#define xen_feature_info_t compat_feature_info_t
>>> -
>>> -CHECK_TYPE(domain_handle);
>> ... this go away without any replacement. Considering they're both
>> char[], that's probably fine, but could do with mentioning in the
>> description.
> 
> I did actually mean to ask about these two, because they're incomplete
> already.
> 
> Why do we CHECK_TYPE(capabilities_info) but define identity aliases for
> compat_extraversion (amongst others) ?
> 
> Is there even a point for having a compat alias of a char array?

To be quite honest, for capabilities_info I don't recall why it wasn't
aliased. For domain_handle I think the reason was that it's declared
outside of this header, and it could conceivably be a more UUID-like
struct.

> I'm tempted to just drop them.  I don't think the check does anything
> useful for us.

As said, I'm pretty much okay with this, but would like you to mention
in the description that this is an intentional change.

Jan