[PATCH v20210209 0/4] tools changes

[PATCH v20210209 0/4] tools changes

[Olaf Hering]

As discussed in the v20210111 thread, split out and redo a few xl and configure.ac specific changes.

Olaf

Olaf Hering (4):
  tools: move CONFIG_DIR and XEN_CONFIG_DIR in paths.m4
  tools: add with-xen-scriptdir configure option
  xl: optionally print timestamps when running xl commands
  xl: disable --debug option for xl migrate

 docs/man/xl.1.pod.in   |  8 ++++----
 m4/paths.m4            | 23 ++++++++++++++---------
 tools/xl/xl.c          | 18 +++++++++++++-----
 tools/xl/xl.h          |  1 +
 tools/xl/xl_cmdtable.c |  1 -
 tools/xl/xl_migrate.c  | 16 +++++++---------
 6 files changed, 39 insertions(+), 28 deletions(-)

[PATCH v20210209 1/4] tools: move CONFIG_DIR and XEN_CONFIG_DIR in paths.m4

[Olaf Hering]

Upcoming changes need to reuse XEN_CONFIG_DIR.

In its current location the assignment happens too late. Move it up
in the file, along with CONFIG_DIR. Their only dependency is
sysconfdir, which may also be adjusted in this file.

No functional change intended.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 m4/paths.m4 | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/m4/paths.m4 b/m4/paths.m4
index 1c107b1a61..a736f57d8d 100644
--- a/m4/paths.m4
+++ b/m4/paths.m4
@@ -34,6 +34,12 @@ if test "x$sysconfdir" = 'x${prefix}/etc' ; then
     esac
 fi
 
+CONFIG_DIR=$sysconfdir
+AC_SUBST(CONFIG_DIR)
+
+XEN_CONFIG_DIR=$CONFIG_DIR/xen
+AC_SUBST(XEN_CONFIG_DIR)
+
 AC_ARG_WITH([initddir],
     AS_HELP_STRING([--with-initddir=DIR],
     [Path to directory with sysv runlevel scripts. [SYSCONFDIR/init.d]]),
@@ -128,15 +134,9 @@ AC_SUBST(XEN_LIB_DIR)
 SHAREDIR=$prefix/share
 AC_SUBST(SHAREDIR)
 
-CONFIG_DIR=$sysconfdir
-AC_SUBST(CONFIG_DIR)
-
 INITD_DIR=$initddir_path
 AC_SUBST(INITD_DIR)
 
-XEN_CONFIG_DIR=$CONFIG_DIR/xen
-AC_SUBST(XEN_CONFIG_DIR)
-
 XEN_SCRIPT_DIR=$XEN_CONFIG_DIR/scripts
 AC_SUBST(XEN_SCRIPT_DIR)
 

Re: [PATCH v20210209 1/4] tools: move CONFIG_DIR and XEN_CONFIG_DIR in paths.m4

[Ian Jackson]

Olaf Hering writes ("[PATCH v20210209 1/4] tools: move CONFIG_DIR and XEN_CONFIG_DIR in paths.m4"):
> Upcoming changes need to reuse XEN_CONFIG_DIR.
> 
> In its current location the assignment happens too late. Move it up
> in the file, along with CONFIG_DIR. Their only dependency is
> sysconfdir, which may also be adjusted in this file.

Reviewed-by: Ian Jackson <iwj@xenproject.org>
Release-Acked-by: Ian Jackson <iwj@xenproject.org>

[PATCH v20210209 2/4] tools: add with-xen-scriptdir configure option

[Olaf Hering]

Some distros plan for fresh installations will have an empty /etc,
whose content will not be controlled by the package manager anymore.

To make this possible, add a knob to configure to allow storing the
hotplug scripts to libexec instead of /etc/xen/scripts.

The current default remains unchanged, which is /etc/xen/scripts.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 m4/paths.m4 | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/m4/paths.m4 b/m4/paths.m4
index a736f57d8d..7be314a3e2 100644
--- a/m4/paths.m4
+++ b/m4/paths.m4
@@ -76,6 +76,14 @@ AC_ARG_WITH([libexec-leaf-dir],
     [libexec_subdir=$withval],
     [libexec_subdir=$PACKAGE_TARNAME])
 
+AC_ARG_WITH([xen-scriptdir],
+    AS_HELP_STRING([--with-xen-scriptdir=DIR],
+    [Path to directory for dom0 hotplug scripts. [SYSCONFDIR/xen/scripts]]),
+    [xen_scriptdir_path=$withval],
+    [xen_scriptdir_path=$XEN_CONFIG_DIR/scripts])
+XEN_SCRIPT_DIR=$xen_scriptdir_path
+AC_SUBST(XEN_SCRIPT_DIR)
+
 AC_ARG_WITH([xen-dumpdir],
     AS_HELP_STRING([--with-xen-dumpdir=DIR],
     [Path to directory for domU crash dumps. [LOCALSTATEDIR/lib/xen/dump]]),
@@ -137,9 +145,6 @@ AC_SUBST(SHAREDIR)
 INITD_DIR=$initddir_path
 AC_SUBST(INITD_DIR)
 
-XEN_SCRIPT_DIR=$XEN_CONFIG_DIR/scripts
-AC_SUBST(XEN_SCRIPT_DIR)
-
 case "$host_os" in
 *freebsd*) XEN_LOCK_DIR=$localstatedir/lib ;;
 *netbsd*) XEN_LOCK_DIR=$rundir_path ;;

Re: [PATCH v20210209 2/4] tools: add with-xen-scriptdir configure option

[Ian Jackson]

Olaf Hering writes ("[PATCH v20210209 2/4] tools: add with-xen-scriptdir configure option"):
> Some distros plan for fresh installations will have an empty /etc,
> whose content will not be controlled by the package manager anymore.
> 
> To make this possible, add a knob to configure to allow storing the
> hotplug scripts to libexec instead of /etc/xen/scripts.
> 
> The current default remains unchanged, which is /etc/xen/scripts.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>
Reviewed-by: Ian Jackson <iwj@xenproject.org>

[PATCH v20210209 3/4] xl: optionally print timestamps when running xl commands

[Olaf Hering]

Add a global option "-T" to xl to enable timestamps in the output from
libxl and libxc. This is most useful with long running commands such
as "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 -T migrate 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.

Note: depending on the command, xl itself also produces other output
which does not go through libxentoollog. As a result such output will
not have timestamps prepended.


This change adds also the missing "-t" flag to "xl help" output.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
---
 docs/man/xl.1.pod.in  |  4 ++++
 tools/xl/xl.c         | 18 +++++++++++++-----
 tools/xl/xl.h         |  1 +
 tools/xl/xl_migrate.c |  3 ++-
 4 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 618c195148..e2176bd696 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -86,6 +86,10 @@ Always use carriage-return-based overwriting for displaying progress
 messages without scrolling the screen.  Without -t, this is done only
 if stderr is a tty.
 
+=item B<-T>
+
+Include timestamps and pid of the xl process in output.
+
 =back
 
 =head1 DOMAIN SUBCOMMANDS
diff --git a/tools/xl/xl.c b/tools/xl/xl.c
index 2a5ddd4390..3a89295802 100644
--- a/tools/xl/xl.c
+++ b/tools/xl/xl.c
@@ -52,6 +52,7 @@ libxl_bitmap global_pv_affinity_mask;
 enum output_format default_output_format = OUTPUT_FORMAT_JSON;
 int claim_mode = 1;
 bool progress_use_cr = 0;
+bool timestamps = 0;
 int max_grant_frames = -1;
 int max_maptrack_frames = -1;
 libxl_domid domid_policy = INVALID_DOMID;
@@ -365,8 +366,9 @@ int main(int argc, char **argv)
     int ret;
     void *config_data = 0;
     int config_len = 0;
+    unsigned int xtl_flags = 0;
 
-    while ((opt = getopt(argc, argv, "+vftN")) >= 0) {
+    while ((opt = getopt(argc, argv, "+vftTN")) >= 0) {
         switch (opt) {
         case 'v':
             if (minmsglevel > 0) minmsglevel--;
@@ -380,6 +382,9 @@ int main(int argc, char **argv)
         case 't':
             progress_use_cr = 1;
             break;
+        case 'T':
+            timestamps = 1;
+            break;
         default:
             fprintf(stderr, "unknown global option\n");
             exit(EXIT_FAILURE);
@@ -394,8 +399,11 @@ int main(int argc, char **argv)
     }
     opterr = 0;
 
-    logger = xtl_createlogger_stdiostream(stderr, minmsglevel,
-        (progress_use_cr ? XTL_STDIOSTREAM_PROGRESS_USE_CR : 0));
+    if (progress_use_cr)
+        xtl_flags |= XTL_STDIOSTREAM_PROGRESS_USE_CR;
+    if (timestamps)
+        xtl_flags |= XTL_STDIOSTREAM_SHOW_DATE | XTL_STDIOSTREAM_SHOW_PID;
+    logger = xtl_createlogger_stdiostream(stderr, minmsglevel, xtl_flags);
     if (!logger) exit(EXIT_FAILURE);
 
     xl_ctx_alloc();
@@ -457,7 +465,7 @@ void help(const char *command)
     struct cmd_spec *cmd;
 
     if (!command || !strcmp(command, "help")) {
-        printf("Usage xl [-vfN] <subcommand> [args]\n\n");
+        printf("Usage xl [-vfNtT] <subcommand> [args]\n\n");
         printf("xl full list of subcommands:\n\n");
         for (i = 0; i < cmdtable_len; i++) {
             printf(" %-19s ", cmd_table[i].cmd_name);
@@ -468,7 +476,7 @@ void help(const char *command)
     } else {
         cmd = cmdtable_lookup(command);
         if (cmd) {
-            printf("Usage: xl [-v%s%s] %s %s\n\n%s.\n\n",
+            printf("Usage: xl [-vtT%s%s] %s %s\n\n%s.\n\n",
                    cmd->modifies ? "f" : "",
                    cmd->can_dryrun ? "N" : "",
                    cmd->cmd_name,
diff --git a/tools/xl/xl.h b/tools/xl/xl.h
index 06569c6c4a..137a29077c 100644
--- a/tools/xl/xl.h
+++ b/tools/xl/xl.h
@@ -269,6 +269,7 @@ extern int run_hotplug_scripts;
 extern int dryrun_only;
 extern int claim_mode;
 extern bool progress_use_cr;
+extern bool timestamps;
 extern xentoollog_level minmsglevel;
 #define minmsglevel_default XTL_PROGRESS
 extern char *lockfile;
diff --git a/tools/xl/xl_migrate.c b/tools/xl/xl_migrate.c
index 0813beb801..b8594f44a5 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -592,9 +592,10 @@ 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%.*s migrate-receive%s%s%s",
                   ssh_command, host,
                   pass_tty_arg ? " -t" : "",
+                  timestamps ? " -T" : "",
                   verbose_len, verbose_buf,
                   daemonize ? "" : " -e",
                   debug ? " -d" : "",

Re: [PATCH v20210209 3/4] xl: optionally print timestamps when running xl commands

[Ian Jackson]

Olaf Hering writes ("[PATCH v20210209 3/4] xl: optionally print timestamps when running xl commands"):
> Add a global option "-T" to xl to enable timestamps in the output from
> libxl and libxc. This is most useful with long running commands such
> as "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 -T migrate 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.
> 
> Note: depending on the command, xl itself also produces other output
> which does not go through libxentoollog. As a result such output will
> not have timestamps prepended.

Reviewed-by: Ian Jackson <iwj@xenproject.org>

This is a new feature so it needs a release risk analysis.

The changes are entirely to xl command line handling.  The worst /
most likely bug would probably be something to do with the way the
logger is created.  Such a bug would be easy to fix.  Or, this patch
could easily be reverted.

Release-Acked-by: Ian Jackson <iwj@xenproject.org>

> This change adds also the missing "-t" flag to "xl help" output.

This part of the commit message talks about -t rather than -T.  I will
fix that on commit.

I'm going to commit the first three now.

Ian.

Re: [PATCH v20210209 3/4] xl: optionally print timestamps when running xl commands

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 326 bytes --]

Am Tue, 9 Feb 2021 16:53:06 +0000
schrieb Ian Jackson <iwj@xenproject.org>:

> This part of the commit message talks about -t rather than -T.  I will
> fix that on commit.

It is really the lowercase t.

01f78a81ae56220dd496a61185ba5dfae30dc2fe did not adjust the output in tools/libxl/xl_cmdimpl.c:help().


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v20210209 3/4] xl: optionally print timestamps when running xl commands

[Ian Jackson]

Olaf Hering writes ("Re: [PATCH v20210209 3/4] xl: optionally print timestamps when running xl commands"):
> Am Tue, 9 Feb 2021 16:53:06 +0000
> schrieb Ian Jackson <iwj@xenproject.org>:
> 
> > This part of the commit message talks about -t rather than -T.  I will
> > fix that on commit.
> 
> It is really the lowercase t.
> 
> 01f78a81ae56220dd496a61185ba5dfae30dc2fe did not adjust the output in tools/libxl/xl_cmdimpl.c:help().

OIC, thanks.

Ian.

[PATCH v20210209 4/4] xl: disable --debug option for xl migrate

[Olaf Hering]

xl migrate --debug used to track every pfn in every batch of pages.

Since commit cfa955591caea5d7ec505cdcbf4442f2d6e889e1 from Xen 4.6 the
debug flag changed meaning from "verify transferred memory during live
migration" to "verify transferred memory in remus/colo". At least xl
will not be able to trigger execution of the verifying code in
send_domain_memory_live anymore.

Remove "--debug" from "xl migrate -h" output.
Remove "--debug" from from xl man page.
Do not send "-d" as potential option for "xl migrate-receive" anymore.
Do not pass the flag LIBXL_SUSPEND_DEBUG anymore to libxl_domain_suspend.
Continue to recognize "--debug" as valid option for "xl migrate".
Continue to recognize "-d" as valid option for "xl migrate-receive".

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  | 15 ++++++---------
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index e2176bd696..b14196ccfe 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -479,10 +479,6 @@ 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<--debug>
-
-Display huge (!) amount of debug information during the migration process.
-
 =item B<-p>
 
 Leave the domain on the receive side paused after migration.
diff --git a/tools/xl/xl_cmdtable.c b/tools/xl/xl_cmdtable.c
index 07f54daabe..150f4cd1d3 100644
--- a/tools/xl/xl_cmdtable.c
+++ b/tools/xl/xl_cmdtable.c
@@ -171,7 +171,6 @@ 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"
-      "--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 b8594f44a5..e4e4f918c7 100644
--- a/tools/xl/xl_migrate.c
+++ b/tools/xl/xl_migrate.c
@@ -177,8 +177,7 @@ static void migrate_do_preamble(int send_fd, int recv_fd, pid_t child,
 }
 
 static void migrate_domain(uint32_t domid, int preserve_domid,
-                           const char *rune, int debug,
-                           const char *override_config_file)
+                           const char *rune, const char *override_config_file)
 {
     pid_t child = -1;
     int rc;
@@ -204,8 +203,6 @@ static void migrate_domain(uint32_t domid, int preserve_domid,
 
     xtl_stdiostream_adjust_flags(logger, XTL_STDIOSTREAM_HIDE_PROGRESS, 0);
 
-    if (debug)
-        flags |= LIBXL_SUSPEND_DEBUG;
     rc = libxl_domain_suspend(ctx, domid, send_fd, flags, NULL);
     if (rc) {
         fprintf(stderr, "migration sender: libxl_domain_suspend failed"
@@ -500,6 +497,7 @@ int main_migrate_receive(int argc, char **argv)
         monitor = 0;
         break;
     case 'd':
+        /* For compatibility with older variants of xl */
         debug = 1;
         break;
     case 'r':
@@ -537,7 +535,7 @@ int main_migrate(int argc, char **argv)
     const char *ssh_command = "ssh";
     char *rune = NULL;
     char *host;
-    int opt, daemonize = 1, monitor = 1, debug = 0, pause_after_migration = 0;
+    int opt, daemonize = 1, monitor = 1, pause_after_migration = 0;
     int preserve_domid = 0;
     static struct option opts[] = {
         {"debug", 0, 0, 0x100},
@@ -566,7 +564,7 @@ int main_migrate(int argc, char **argv)
         preserve_domid = 1;
         break;
     case 0x100: /* --debug */
-        debug = 1;
+        /* ignored for compatibility with older variants of xl */
         break;
     case 0x200: /* --live */
         /* ignored for compatibility with xm */
@@ -592,17 +590,16 @@ int main_migrate(int argc, char **argv)
         } else {
             verbose_len = (minmsglevel_default - minmsglevel) + 2;
         }
-        xasprintf(&rune, "exec %s %s xl%s%s%.*s migrate-receive%s%s%s",
+        xasprintf(&rune, "exec %s %s xl%s%s%.*s migrate-receive%s%s",
                   ssh_command, host,
                   pass_tty_arg ? " -t" : "",
                   timestamps ? " -T" : "",
                   verbose_len, verbose_buf,
                   daemonize ? "" : " -e",
-                  debug ? " -d" : "",
                   pause_after_migration ? " -p" : "");
     }
 
-    migrate_domain(domid, preserve_domid, rune, debug, config_filename);
+    migrate_domain(domid, preserve_domid, rune, config_filename);
     return EXIT_SUCCESS;
 }
 

Re: [PATCH v20210209 4/4] xl: disable --debug option for xl migrate

[Ian Jackson]

Olaf Hering writes ("[PATCH v20210209 4/4] xl: disable --debug option for xl migrate"):
> xl migrate --debug used to track every pfn in every batch of pages.
> 
> Since commit cfa955591caea5d7ec505cdcbf4442f2d6e889e1 from Xen 4.6 the
> debug flag changed meaning from "verify transferred memory during live
> migration" to "verify transferred memory in remus/colo". At least xl
> will not be able to trigger execution of the verifying code in
> send_domain_memory_live anymore.
> 
> Remove "--debug" from "xl migrate -h" output.
> Remove "--debug" from from xl man page.
> Do not send "-d" as potential option for "xl migrate-receive" anymore.
> Do not pass the flag LIBXL_SUSPEND_DEBUG anymore to libxl_domain_suspend.
> Continue to recognize "--debug" as valid option for "xl migrate".
> Continue to recognize "-d" as valid option for "xl migrate-receive".

It seems to me that something is definitely a bug here but I want to
understand from Andy what the best thing to do is.  I'm hesitant to
release-ack removing this at this stage.

Wouldn't it be better to just fix the docs like in your previously
suggested patch ?

Also, Olaf, please CC Andy on these migration-related patches.

Thanks,
Ian.

Re: [PATCH v20210209 4/4] xl: disable --debug option for xl migrate

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

Am Tue, 9 Feb 2021 17:12:28 +0000
schrieb Ian Jackson <iwj@xenproject.org>:

> Also, Olaf, please CC Andy on these migration-related patches.

Can this be automated via MAINTAINERS, so that scripts/get_maintainer.pl addresses the people who feel responsible for it? Right now it ends up in your queue due to 'tools/*'.


Olaf 

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v20210209 4/4] xl: disable --debug option for xl migrate

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 915 bytes --]

Am Tue, 9 Feb 2021 17:12:28 +0000
schrieb Ian Jackson <iwj@xenproject.org>:

> It seems to me that something is definitely a bug here but I want to
> understand from Andy what the best thing to do is.  I'm hesitant to
> release-ack removing this at this stage.
> 
> Wouldn't it be better to just fix the docs like in your previously
> suggested patch ?

To make that initial patch accurate, this additional change is required:

--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -752,7 +752,7 @@ static int send_domain_memory_live(struct xc_sr_context *ctx)
     if ( rc )
         goto out;
 
-    if ( ctx->save.debug && ctx->stream_type != XC_STREAM_PLAIN )
+    if ( ctx->save.debug )
     {
         rc = verify_frames(ctx);
         if ( rc )

Otherwise there is no way for "xl" to trigger a call into verify_frames.

I will test this patch today.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v20210209 4/4] xl: disable --debug option for xl migrate

[Olaf Hering]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

Am Wed, 10 Feb 2021 10:06:06 +0100
schrieb Olaf Hering <olaf@aepfle.de>:

> -    if ( ctx->save.debug && ctx->stream_type != XC_STREAM_PLAIN )
> +    if ( ctx->save.debug )

This will do the verification, and finds many errors:

2021-02-10 02:37:03 MST [2149] xc: error: verify pfn 0xfda9 failed (type 0): Internal error

As Andrew said elsewhere, something writes into the memory of the idle, but suspended domU. Likely the netback, since it can not know that the domU will never come back.

No idea if verify_frames would have ways to figure out what purpose a given pfn really has.


Olaf

[-- Attachment #2: Digitale Signatur von OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]