From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yang Hongyang Subject: Re: [PATCH v2 COLOPre 10/13] tools/libxl: Add back channel to allow migration target send data back Date: Mon, 15 Jun 2015 09:33:16 +0800 Message-ID: <557E2B5C.8070208@cn.fujitsu.com> References: <1433734997-26570-1-git-send-email-yanghy@cn.fujitsu.com> <1433734997-26570-11-git-send-email-yanghy@cn.fujitsu.com> <20150612125429.GO14606@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150612125429.GO14606@zion.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: Wei Liu Cc: ian.campbell@citrix.com, wency@cn.fujitsu.com, andrew.cooper3@citrix.com, yunhong.jiang@intel.com, eddie.dong@intel.com, xen-devel@lists.xen.org, guijianfeng@cn.fujitsu.com, rshriram@cs.ubc.ca, ian.jackson@eu.citrix.com List-Id: xen-devel@lists.xenproject.org On 06/12/2015 08:54 PM, Wei Liu wrote: > On Mon, Jun 08, 2015 at 11:43:14AM +0800, Yang Hongyang wrote: >> From: Wen Congyang >> >> In colo mode, slave needs to send data to master, but the io_fd >> only can be written in master, and only can be read in slave. >> Save recv_fd in domain_suspend_state, and send_fd in >> domain_create_state. >> > > You failed to mention in commit message new structures are introduced in > IDL. > >> Signed-off-by: Wen Congyang >> Signed-off-by: Yang Hongyang >> --- >> tools/libxl/libxl.c | 2 +- >> tools/libxl/libxl_create.c | 14 ++++++++++---- >> tools/libxl/libxl_internal.h | 2 ++ >> tools/libxl/libxl_types.idl | 7 +++++++ >> tools/libxl/xl_cmdimpl.c | 7 +++++++ > > You also need to add LIBXL_HAVE in libxl.h. > >> 5 files changed, 27 insertions(+), 5 deletions(-) >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index 5c843c2..36b97fe 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -832,7 +832,7 @@ int libxl_domain_remus_start(libxl_ctx *ctx, libxl_domain_remus_info *info, >> dss->callback = remus_failover_cb; >> dss->domid = domid; >> dss->fd = send_fd; >> - /* TODO do something with recv_fd */ >> + dss->recv_fd = recv_fd; >> dss->type = type; >> dss->live = 1; >> dss->debug = 0; >> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c >> index 86384d2..bd8149c 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, >> + int send_fd, int checkpointed_stream, >> const libxl_asyncop_how *ao_how, >> const libxl_asyncprogress_how *aop_console_how) >> { >> @@ -1591,6 +1591,7 @@ 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; >> + cdcs->dcs.send_fd = send_fd; >> cdcs->dcs.callback = domain_create_cb; >> cdcs->dcs.checkpointed_stream = checkpointed_stream; >> libxl__ao_progress_gethow(&cdcs->dcs.aop_console_how, aop_console_how); >> @@ -1619,7 +1620,7 @@ int libxl_domain_create_new(libxl_ctx *ctx, libxl_domain_config *d_config, >> const libxl_asyncop_how *ao_how, >> const libxl_asyncprogress_how *aop_console_how) >> { >> - return do_domain_create(ctx, d_config, domid, -1, 0, >> + return do_domain_create(ctx, d_config, domid, -1, -1, 0, >> ao_how, aop_console_how); >> } >> >> @@ -1629,7 +1630,12 @@ int libxl_domain_create_restore(libxl_ctx *ctx, libxl_domain_config *d_config, >> const libxl_asyncop_how *ao_how, >> const libxl_asyncprogress_how *aop_console_how) >> { >> - return do_domain_create(ctx, d_config, domid, restore_fd, >> + int send_fd = -1; >> + >> + if (params->checkpointed_stream == LIBXL_CHECKPOINTED_STREAM_COLO) >> + send_fd = params->send_fd; >> + >> + return do_domain_create(ctx, d_config, domid, restore_fd, send_fd, >> params->checkpointed_stream, ao_how, aop_console_how); >> } >> >> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h >> index fbbae93..6d214b5 100644 >> --- a/tools/libxl/libxl_internal.h >> +++ b/tools/libxl/libxl_internal.h >> @@ -2874,6 +2874,7 @@ struct libxl__domain_save_state { >> >> uint32_t domid; >> int fd; >> + int recv_fd; >> libxl_domain_type type; >> int live; >> int debug; >> @@ -3143,6 +3144,7 @@ struct libxl__domain_create_state { >> libxl_domain_config *guest_config; >> libxl_domain_config guest_config_saved; /* vanilla config */ >> int restore_fd; >> + int send_fd; >> libxl__domain_create_cb *callback; >> libxl_asyncprogress_how aop_console_how; >> /* private to domain_create */ >> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl >> index 23f27d4..8a3d7ba 100644 >> --- a/tools/libxl/libxl_types.idl >> +++ b/tools/libxl/libxl_types.idl >> @@ -198,6 +198,12 @@ libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [ >> (3, "reference_tsc"), >> ]) >> >> +libxl_checkpointed_stream = Enumeration("checkpointed_stream", [ >> + (0, "NONE"), >> + (1, "REMUS"), >> + (2, "COLO"), >> + ], init_val = 0) > > The default init_val is 0 so you don't need to write it down. Okay. > >> + >> # >> # Complex libxl types >> # >> @@ -346,6 +352,7 @@ libxl_domain_create_info = Struct("domain_create_info",[ >> >> libxl_domain_restore_params = Struct("domain_restore_params", [ >> ("checkpointed_stream", integer), >> + ("send_fd", integer), > > I'm not entirely sure if we want to bury an extra argument here. > > After looking at code I think you're trying to work around API > limitation. I think we are safe to extend the API -- we've already done > that before. See libxl.h around line 990. > > Ian and Ian, what do you think? > >> ]) >> >> libxl_domain_sched_params = Struct("domain_sched_params",[ >> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c >> index c858068..adfadd1 100644 >> --- a/tools/libxl/xl_cmdimpl.c >> +++ b/tools/libxl/xl_cmdimpl.c > > I would also suggest you rename the last argument of migrate_receive in > this file from "remus" to "checkpointed_stream_type" since the semantics > of that parameter has changed. I've renamed it to "checkpointed". > > Wei. > . > -- Thanks, Yang.