| Message ID | 20210111174224.24714-8-olaf@aepfle.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | leftover from 2020 | expand |
Olaf Hering writes ("[PATCH v20210111 07/39] xl: optionally print timestamps during xl migrate"):
> During 'xl -v.. migrate domU host' a large amount of debug is generated.
> It is difficult to map each line to the sending and receiving side.
> Also the time spent for migration is not reported.
>
> With 'xl migrate -T domU host' both sides will print timestamps and
> also the pid of the invoked xl process to make it more obvious which
> side produced a given log line.
>
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
This is from October and ought to have been reviewed much sooner.
Sorry.
With my maintainer hat on: this is a useful development but I am not
sure about the choice of -T, and the choice to make this a
migrate-specific option. Most unix things that have an option to
print timestamps use `-t`. But we have already stolen `-t` as a
global option for "prinht CR as well as LF". Hrmf.
Under the circumstances -T for migrate seems a plausible and
conservative choice. I think maybe we should maybe add a global -T
later.
Reviewed-by: Ian Jackson <iwj@xenproject.org>
Now I have to decide what to do about it for 4.15.
The downside risks I see from reading the patch are:
* The CLI API choice is being made in a hurry.
* We might break something. This actually seems quite unlikely since
I have reviewed the code changes in detail.
I wonder if I can get a quick second option from someone on the API
question. Using up a single letter option is something I don't want
to just rush into.
Ian.
Am Mon, 8 Feb 2021 16:22:54 +0000 schrieb Ian Jackson <iwj@xenproject.org>: > I wonder if I can get a quick second option from someone on the API > question. Using up a single letter option is something I don't want > to just rush into. Maybe put "-T" or "-t" into xl.c:main() instead, so that every command prints timestamp+pid? For everything beside "migrate" it will only be useful along with a couple of "-v" to enable global debug output. Olaf
Olaf Hering writes ("Re: [PATCH v20210111 07/39] xl: optionally print timestamps during xl migrate"):
> Am Mon, 8 Feb 2021 16:22:54 +0000
> schrieb Ian Jackson <iwj@xenproject.org>:
>
> > I wonder if I can get a quick second option from someone on the API
> > question. Using up a single letter option is something I don't want
> > to just rush into.
>
> Maybe put "-T" or "-t" into xl.c:main() instead, so that every command prints timestamp+pid?
Yes, I think following some informal irc discussions that that would
be best.
> For everything beside "migrate" it will only be useful along with a couple of "-v" to enable global debug output.
Right.
Ian.
Am Mon, 8 Feb 2021 16:22:54 +0000 schrieb Ian Jackson <iwj@xenproject.org>: > With my maintainer hat on: this is a useful development but I am not > sure about the choice of -T, and the choice to make this a > migrate-specific option. Most unix things that have an option to > print timestamps use `-t`. But we have already stolen `-t` as a > global option for "prinht CR as well as LF". Hrmf. Was 'xl -t' intentionally left out of xl.c:help()? It is only mentioned in the xl man page. If yes, my change will also omit it. If no, I will add it along with the global -T option. Olaf
diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in index df98adc9e4..494a84ee13 100644 --- a/docs/man/xl.1.pod.in +++ b/docs/man/xl.1.pod.in @@ -475,6 +475,10 @@ domain. See the corresponding option of the I<create> subcommand. Send the specified <config> file instead of the file used on creation of the domain. +=item B<-T> + +Include timestamps in output. + =item B<--debug> Display huge (!) amount of debug information during the migration process. diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c index 37710880d3..da0473ddfb 100644 --- a/tools/xl/xl_cmdtable.c +++ b/tools/xl/xl_cmdtable.c @@ -167,6 +167,7 @@ struct cmd_spec cmd_table[] = { " migrate-receive [-d -e]\n" "-e Do not wait in the background (on <host>) for the death\n" " of the domain.\n" + "-T Show timestamps during the migration process.\n" "--debug Print huge (!) amount of debug during the migration process.\n" "-p Do not unpause domain after migrating it.\n" "-D Preserve the domain id" diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c index 0813beb801..856a6e2be1 100644 --- a/tools/xl/xl_migrate.c +++ b/tools/xl/xl_migrate.c @@ -32,6 +32,8 @@ #ifndef LIBXL_HAVE_NO_SUSPEND_RESUME +static bool timestamps; + static pid_t create_migration_child(const char *rune, int *send_fd, int *recv_fd) { @@ -187,6 +189,7 @@ static void migrate_domain(uint32_t domid, int preserve_domid, char rc_buf; uint8_t *config_data; int config_len, flags = LIBXL_SUSPEND_LIVE; + unsigned xtl_flags = XTL_STDIOSTREAM_HIDE_PROGRESS; save_domain_core_begin(domid, preserve_domid, override_config_file, &config_data, &config_len); @@ -202,7 +205,9 @@ static void migrate_domain(uint32_t domid, int preserve_domid, migrate_do_preamble(send_fd, recv_fd, child, config_data, config_len, rune); - xtl_stdiostream_adjust_flags(logger, XTL_STDIOSTREAM_HIDE_PROGRESS, 0); + if (timestamps) + xtl_flags |= XTL_STDIOSTREAM_SHOW_DATE | XTL_STDIOSTREAM_SHOW_PID; + xtl_stdiostream_adjust_flags(logger, xtl_flags, 0); if (debug) flags |= LIBXL_SUSPEND_DEBUG; @@ -328,6 +333,11 @@ static void migrate_receive(int debug, int daemonize, int monitor, char rc_buf; char *migration_domname; struct domain_create dom_info; + unsigned xtl_flags = 0; + + if (timestamps) + xtl_flags |= XTL_STDIOSTREAM_SHOW_DATE | XTL_STDIOSTREAM_SHOW_PID; + xtl_stdiostream_adjust_flags(logger, xtl_flags, 0); signal(SIGPIPE, SIG_IGN); /* if we get SIGPIPE we'd rather just have it as an error */ @@ -491,7 +501,7 @@ int main_migrate_receive(int argc, char **argv) COMMON_LONG_OPTS }; - SWITCH_FOREACH_OPT(opt, "Fedrp", opts, "migrate-receive", 0) { + SWITCH_FOREACH_OPT(opt, "FedrpT", opts, "migrate-receive", 0) { case 'F': daemonize = 0; break; @@ -517,6 +527,9 @@ int main_migrate_receive(int argc, char **argv) case 'p': pause_after_migration = 1; break; + case 'T': + timestamps = 1; + break; } if (argc-optind != 0) { @@ -545,7 +558,7 @@ int main_migrate(int argc, char **argv) COMMON_LONG_OPTS }; - SWITCH_FOREACH_OPT(opt, "FC:s:epD", opts, "migrate", 2) { + SWITCH_FOREACH_OPT(opt, "FC:s:eTpD", opts, "migrate", 2) { case 'C': config_filename = optarg; break; @@ -559,6 +572,9 @@ int main_migrate(int argc, char **argv) daemonize = 0; monitor = 0; break; + case 'T': + timestamps = 1; + break; case 'p': pause_after_migration = 1; break; @@ -592,11 +608,12 @@ int main_migrate(int argc, char **argv) } else { verbose_len = (minmsglevel_default - minmsglevel) + 2; } - xasprintf(&rune, "exec %s %s xl%s%.*s migrate-receive%s%s%s", + xasprintf(&rune, "exec %s %s xl%s%.*s migrate-receive%s%s%s%s", ssh_command, host, pass_tty_arg ? " -t" : "", verbose_len, verbose_buf, daemonize ? "" : " -e", + timestamps ? " -T" : "", debug ? " -d" : "", pause_after_migration ? " -p" : ""); }
During 'xl -v.. migrate domU host' a large amount of debug is generated. It is difficult to map each line to the sending and receiving side. Also the time spent for migration is not reported. With 'xl migrate -T domU host' both sides will print timestamps and also the pid of the invoked xl process to make it more obvious which side produced a given log line. Signed-off-by: Olaf Hering <olaf@aepfle.de> --- docs/man/xl.1.pod.in | 4 ++++ tools/xl/xl_cmdtable.c | 1 + tools/xl/xl_migrate.c | 25 +++++++++++++++++++++---- 3 files changed, 26 insertions(+), 4 deletions(-)