Re: [Xen-devel] [RFC PATCH v2 5/7] Add cbs parameter support and removed sedf parameters with a LIBXL_API_VERSION gate from libxl.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2014-07-09 at 16:55 -0400, Josh Whitehead wrote:
> From: Robbie VanVossen <robert.vanvossen@xxxxxxxxxxxxxxx>
> 
> We made an attempt at doing the versioning gate in this file.  Please let us
> know if further changes are needed, and where they are needed to properly 
> guard
> the API.

I previously said that I thought it was fine to remove the obsolete SEDF
stuff from the libxl API.

However I mentioned this to the other maintainers and they disagreed
with me. The feeling is that an old application which used the old SEDF
parameters should continue to build but that trying to use those
parameters should result in an error. I'm sorry for steering you wrong
here.

However looking at the diff of libxl_types.idl it seems like you haven't
removed anything from the API struct so perhaps this is already close to
ok. I think all you need to do is arrange to return an error when
someone tries to use the old parameters.

> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> index 900b8d4..ca8c1c5 100644
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -4932,13 +4932,21 @@ static int sched_sedf_domain_get(libxl__gc *gc, 
> uint32_t domid,
>  {
>      uint64_t period;
>      uint64_t slice;
> +#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION <= 0x040400

There shouldn't be any use of LIBXL_API_VERSION in libxl*.c files.

The libxl code always implement the latest interface. The purpose of
LIBXL_API_VERSION is so that in libxl.h we can provide shims to bridge
the gap between the version that the application has requested and the
latest implementation. 

There will only be one libxl.so binary, but it should be possible to
compile and link an old application using an old LIBXL_API_VERSION
against the libxl.h header and have it still work, without heeding to
change the library.

See for example the handling of libxl_set_vcpuaffinity. In the 4.5 cycle
this gained a new parameter (a soft affinity bitmap). The compat shim is
just:
        #if defined (LIBXL_API_VERSION) && LIBXL_API_VERSION < 0x040500
        
        #define libxl_set_vcpuaffinity(ctx, domid, vcpuid, map) \
            libxl_set_vcpuaffinity((ctx), (domid), (vcpuid), (map), NULL)
passing NULL for the new param. The implementation of
libxl_set_vcpuaffinity has to handle NULL for this parameter (which it
did anyway) bit it is not (and cannot be) affected by LIBXL_API_VERSION.

I think you can for the most part just remove any code which is behind a
#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION <= 0x040400 and
instead add checks like
        if (foo->slice != LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT) 
                return ERROR_INVAL; /* deprecated option used */
at the entry points.

Ian (J) -- is this the sort of thing you meant?

> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index 52f1aa9..d02380e 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -294,6 +294,7 @@ libxl_domain_sched_params = Struct("domain_sched_params",[
>      ("slice",        integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_SLICE_DEFAULT'}),
>      ("latency",      integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_LATENCY_DEFAULT'}),
>      ("extratime",    integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_EXTRATIME_DEFAULT'}),
> +    ("soft",         integer, {'init_val': 
> 'LIBXL_DOMAIN_SCHED_PARAM_SOFT_DEFAULT'}),
>      ])

Please could you also add some comments to indicate which fields are no
longer supported.

Please also add a #define LIBXL_HAVE_* to indicate the presence of this
new field (and by implication the unavailability/uselessness of the old
ones). There are some examples in the header already.

>  libxl_domain_build_info = Struct("domain_build_info",[
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 5195914..1f6f04b 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -838,10 +838,15 @@ static void parse_config_data(const char *config_source,
>          b_info->sched_params.period = l;
>      if (!xlu_cfg_get_long (config, "slice", &l, 0))
>          b_info->sched_params.slice = l;
> +#if defined(LIBXL_API_VERSION) && LIBXL_API_VERSION <= 0x040400

Since xl is intree it doesn't need to use LIBXL_API_VERSION, it can just
use the new interface. So on the xl side you should strip out everything
behind LIBXL_API_VERSION <= 0x040400.

The purpose of LIBXL_API_VERSION is that the application sets it while
including libxl.h and then proceeds to use that version's interface
without the need for all these ifdefs on the application side, so what
you've done is backwards.

Ian.


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