From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 03/27] tools/libxl: Stash all restore parameters in domain_create_state Date: Tue, 16 Jun 2015 15:09:28 +0100 Message-ID: <55802E18.8070903@citrix.com> References: <1434375880-30914-1-git-send-email-andrew.cooper3@citrix.com> <1434375880-30914-4-git-send-email-andrew.cooper3@citrix.com> <1434461873.13744.159.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1434461873.13744.159.camel@citrix.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: Wei Liu , Yang Hongyang , Ian Jackson , Xen-devel List-Id: xen-devel@lists.xenproject.org On 16/06/15 14:37, Ian Campbell wrote: > On Mon, 2015-06-15 at 14:44 +0100, Andrew Cooper wrote: >> Shortly more parameters will appear, and this saves unboxing each one. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper >> CC: Ian Campbell >> CC: Ian Jackson >> CC: Wei Liu >> --- >> tools/libxl/libxl_create.c | 12 ++++++------ >> tools/libxl/libxl_internal.h | 2 +- >> tools/libxl/libxl_save_callout.c | 2 +- >> 3 files changed, 8 insertions(+), 8 deletions(-) >> >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 86384d2..385891c 100644 >> --- a/tools/libxl/libxl_create.c >> +++ b/tools/libxl/libxl_create.c >> @@ -1577,8 +1577,8 @@ static void domain_create_cb(libxl__egc *egc, >> int rc, uint32_t domid); >> >> static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, >> - uint32_t *domid, >> - int restore_fd, int checkpointed_stream, >> + uint32_t *domid, int restore_fd, >> + const libxl_domain_restore_params *params, >> const libxl_asyncop_how *ao_how, >> const libxl_asyncprogress_how *aop_console_how) >> { >> @@ -1591,8 +1591,8 @@ static int do_domain_create(libxl_ctx *ctx, libxl_domain_config *d_config, >> libxl_domain_config_init(&cdcs->dcs.guest_config_saved); >> libxl_domain_config_copy(ctx, &cdcs->dcs.guest_config_saved, d_config); >> cdcs->dcs.restore_fd = restore_fd; >> + if (params) cdcs->dcs.restore_params = *params; > Is this eventually going to become non-optional? I think not and its > validity is entirely intertwined with the validity of restore_fd (as I > suspect it was before, but I've not checked). > > Perhaps an error check to that effect would be useful? It is mandatory for restore, and currently unused for plain create. restore_fd being > -1 does appear to be the canonical switch between a restore and a create, so should be the qualification of validity. > > Anyway, I think what you've done here is correct, so: > Acked-by: Ian Campbell > > [...] >> @@ -3122,11 +3122,11 @@ struct libxl__domain_create_state { >> libxl_domain_config *guest_config; >> libxl_domain_config guest_config_saved; /* vanilla config */ >> int restore_fd; >> + libxl_domain_restore_params restore_params; >> libxl__domain_create_cb *callback; >> libxl_asyncprogress_how aop_console_how; >> /* private to domain_create */ >> int guest_domid; >> - int checkpointed_stream; > This has, in effect moved from "private to domain_create" to "filled in > by user", I don't think the change here has actually changed its status, > but I suspect it was wrong before (alternatively restore_fd is in the > wrong place instead). I think it was wrong before. It was always a caller-provided parameter, albeit implicit by virtue of essentially being a "remus" boolean. ~Andrew