From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [RFC PATCH v2 5/7] Add cbs parameter support and removed sedf parameters with a LIBXL_API_VERSION gate from libxl. Date: Thu, 10 Jul 2014 15:09:45 +0100 Message-ID: <1405001385.24434.20.camel@kazak.uk.xensource.com> References: <1404939348-4926-1-git-send-email-josh.whitehead@dornerworks.com> <1404939348-4926-6-git-send-email-josh.whitehead@dornerworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1404939348-4926-6-git-send-email-josh.whitehead@dornerworks.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Josh Whitehead Cc: Stefano Stabellini , George Dunlap , Dario Faggioli , Ian Jackson , Robert VanVossen , Xen-devel , Nathan Studer List-Id: xen-devel@lists.xenproject.org On Wed, 2014-07-09 at 16:55 -0400, Josh Whitehead wrote: > From: Robbie VanVossen > > 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.