Re: [Xen-devel] [PATCH v4 01/34] compat/x86: Remove unncessary #define.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.03.16 at 01:44, <konrad@xxxxxxxxxx> wrote:
> On Wed, Mar 16, 2016 at 05:08:30AM -0600, Jan Beulich wrote:
>> >>> On 15.03.16 at 18:56, <konrad.wilk@xxxxxxxxxx> wrote:
>> > It is not used.
>> 
>> Consistently please - either keep them all (just to cover the case
>> that they might get used) or remove them all: xen_compile_info,
>> xen_changeset_info, etc are all unused too. Otoh
>> xennmi_callback is used, but xennmi_callback_t isn't. Which to me
>> suggests that we should leave this alone.
> 
> Oddly enough taking an cleaver to it was OK.
> 
> From 7e3ed6faed6e083f27ad6be947ac528c3eaba9a1 Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Date: Wed, 2 Mar 2016 12:50:32 -0500
> Subject: [PATCH v4 02/35] compat/x86: Remove unncessary #defines.
> 
> They are not used.

This now goes too far.

> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Hence this can't stay.

> ---
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Tim Deegan <tim@xxxxxxx>
> 
> v2: Remove a lot more of them.

(Side note: Not only this time round I notice versioning mixup in
your patches: The subject says v4 here, yet the update to it
above says v2. In the previous round of the xSplice series - v3 -
I seem to recall there were patches showing a "history" up to
something like v9. I think in cases of such heavy mixing it should
be the patch with the oldest history that determines the version
of the entire series.)

> --- a/xen/common/compat/kernel.c
> +++ b/xen/common/compat/kernel.c
> @@ -18,30 +18,22 @@ asm(".file \"" __FILE__ "\"");
>  
>  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);
>  
> -#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);
>  
> -#define xennmi_callback compat_nmi_callback
> -#define xennmi_callback_t compat_nmi_callback_t

The former definitely is being used; not getting a compilation
error here is not a sign of things being right. That's another
reason why - as I had suggested already - we'd probably
better leave things as they are: Introduction of uses of the
now removed identifiers might otherwise break compat code
without anyone noticing right away.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel