| Message ID | 1465210332-25440-2-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> The original code is bogus because the common case is no HA enabled.
> Setting ha variable at the beginning is not very useful.
This seems like a plausible cleanup but I don't understand why you say
the original code is `bogus'. Is it wrong ? If so you don't say how.
Ian.
On Tue, Jun 14, 2016 at 11:18:16AM +0100, Ian Jackson wrote: > Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"): > > The original code is bogus because the common case is no HA enabled. > > Setting ha variable at the beginning is not very useful. > > This seems like a plausible cleanup but I don't understand why you say > the original code is `bogus'. Is it wrong ? If so you don't say how. > What I meant is "the default state should be no HA enabled", hence if we don't move the ha variable into a narrower scope it should have been initialised to NULL first. It is not a bug though. Wei. > Ian.
Wei Liu writes ("Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> On Tue, Jun 14, 2016 at 11:18:16AM +0100, Ian Jackson wrote:
> > Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"):
> > > The original code is bogus because the common case is no HA enabled.
> > > Setting ha variable at the beginning is not very useful.
> >
> > This seems like a plausible cleanup but I don't understand why you say
> > the original code is `bogus'. Is it wrong ? If so you don't say how.
>
> What I meant is "the default state should be no HA enabled", hence if we
> don't move the ha variable into a narrower scope it should have been
> initialised to NULL first. It is not a bug though.
IC. Perhaps you'd like to clarify the commit message ?
Whether or not you do,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
On Tue, Jun 14, 2016 at 03:15:19PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"): > > On Tue, Jun 14, 2016 at 11:18:16AM +0100, Ian Jackson wrote: > > > Wei Liu writes ("[PATCH 1/6] xl: remus/colo: only initialise ha variable when necessary"): > > > > The original code is bogus because the common case is no HA enabled. > > > > Setting ha variable at the beginning is not very useful. > > > > > > This seems like a plausible cleanup but I don't understand why you say > > > the original code is `bogus'. Is it wrong ? If so you don't say how. > > > > What I meant is "the default state should be no HA enabled", hence if we > > don't move the ha variable into a narrower scope it should have been > > initialised to NULL first. It is not a bug though. > > IC. Perhaps you'd like to clarify the commit message ? Sure, I will incorporate my reply above to the commit message. > Whether or not you do, > > Acked-by: Ian Jackson <ian.jackson@eu.citrix.com> Thanks. Wei.
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c index d8530f0..c33691c 100644 --- a/tools/libxl/xl_cmdimpl.c +++ b/tools/libxl/xl_cmdimpl.c @@ -4752,8 +4752,6 @@ static void migrate_receive(int debug, int daemonize, int monitor, char rc_buf; char *migration_domname; struct domain_create dom_info; - const char *ha = checkpointed == LIBXL_CHECKPOINTED_STREAM_COLO ? - "COLO" : "Remus"; signal(SIGPIPE, SIG_IGN); /* if we get SIGPIPE we'd rather just have it as an error */ @@ -4788,6 +4786,9 @@ static void migrate_receive(int debug, int daemonize, int monitor, switch (checkpointed) { case LIBXL_CHECKPOINTED_STREAM_REMUS: case LIBXL_CHECKPOINTED_STREAM_COLO: + { + const char *ha = checkpointed == LIBXL_CHECKPOINTED_STREAM_COLO ? + "COLO" : "Remus"; /* If we are here, it means that the sender (primary) has crashed. * TODO: Split-Brain Check. */ @@ -4824,6 +4825,7 @@ static void migrate_receive(int debug, int daemonize, int monitor, ha, common_domname, domid, rc); exit(rc ? EXIT_FAILURE : EXIT_SUCCESS); + } default: /* do nothing */ break;
The original code is bogus because the common case is no HA enabled. Setting ha variable at the beginning is not very useful. Move ha to the scope where it is used. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- tools/libxl/xl_cmdimpl.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)