From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v5 RFC] tools/migrate: Fix regression when migrating from older version of Xen Date: Thu, 26 Sep 2013 17:11:44 +0100 Message-ID: <52445CC0.9010505@citrix.com> References: <1380203111-32083-1-git-send-email-andrew.cooper3@citrix.com> <1380208284.29483.116.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1380208284.29483.116.camel@kazak.uk.xensource.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: Ian Campbell Cc: Ian Jackson , Xen-devel List-Id: xen-devel@lists.xenproject.org On 26/09/13 16:11, Ian Campbell wrote: > Just commenting on the API compatibility bits here. > > Can you CC Shriram (remus guy) in the next iteration please. > > On Thu, 2013-09-26 at 14:45 +0100, Andrew Cooper wrote: >> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h >> index 4cab294..28af9c2 100644 >> --- a/tools/libxl/libxl.h >> +++ b/tools/libxl/libxl.h >> @@ -355,6 +355,20 @@ >> */ >> #define LIBXL_HAVE_SPICE_VDAGENT 1 >> >> +/* >> + * LIBXL_HAVE_LIBXL_DOMAIN_CREATE_RESTORE_CHECKPOINTED 1 > LIBXL_HAVE_DOMAIN_CREATE... (no need to repeat the LIBXL, or at least we > don't elsewhere) > >> + * If this is defined, libxl_domain_create_restore()'s API has changed to >> + * include a checkpointed_stream flag which gets passed down to libxc. >> + */ >> +#define LIBXL_HAVE_LIBXL_DOMAIN_CREATE_RESTORE_CHECKPOINTED 1 >> +#if LIBXL_API_VERSION == 0x040200 || LIBXL_API_VERSION == 0x040300 > <= 0x040300 is fine I think. > >> +# define libxl_domain_create_restore libxl_domain_create_restore_V040200 > I prefer "...restore_0x040200" since it looks like the #define. > >> +#else >> +# define libxl_domain_create_restore libxl_domain_create_restore_V040400 >> +#endif > I think you can omit the #else clause and just call the new function > libxl_domain_create_restore. libxl will never set LIBXL_API_VERSION when > building itself. > >> +int libxl_domain_create_restore_V040200( >> + libxl_ctx *ctx, libxl_domain_config *d_config, >> + uint32_t *domid, int restore_fd, >> + const libxl_asyncop_how *ao_how, >> + const libxl_asyncprogress_how *aop_console_how) >> + LIBXL_EXTERNAL_CALLERS_ONLY; >> +int libxl_domain_create_restore_V040400( >> + libxl_ctx *ctx, libxl_domain_config *d_config, >> + uint32_t *domid, int restore_fd, >> + const libxl_asyncop_how *ao_how, >> + const libxl_asyncprogress_how *aop_console_how, >> + int checkpointed_stream) >> + LIBXL_EXTERNAL_CALLERS_ONLY; > We seem to habitually put the ao_how and aop_ stuff last, after the > actual "arguments". > > Did we consider adding a libxl_domain_restore_params struct to contain > the checkpointed flag? That would make this interface more easily > extensible in the future. That was considered, but it would involve playing with the IDL (with which I have no experience), and involve substantially more changes for both in and out-of-tree consumers of the function. > >> +int libxl_domain_create_restore_V040200( >> + libxl_ctx *ctx, libxl_domain_config *d_config, >> + uint32_t *domid, int restore_fd, >> + const libxl_asyncop_how *ao_how, >> + const libxl_asyncprogress_how *aop_console_how) >> +{ >> + return do_domain_create(ctx, d_config, domid, restore_fd, >> + ao_how, aop_console_how, 0); >> +} > Could static inline this as a call to the non suffixed version I think? > > Cannot do both this and drop the #else clause. (unless I have missed something obvious) ~Andrew