[Qemu-devel] QMP, HMP: introduce 'writeconfig' command

[Qemu-devel] QMP, HMP: introduce 'writeconfig' command

[Vadim Galitsyn]

Hi Guys,

This thread is a continuation of discussion started in
http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03182.html.

This series introduces ‘writeconfig’ command support for QMP and HMP monitors. This functionality might be useful for live migration for cases when guest configuration was modified in runtime (for example as a result of hot- plug/unplug operations) and actual Qemu command line no longer reflects setup exposed to guest.

Original series has ‘qemu_opts’ patch as well (http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03183.html) because HMP’s ‘object_add’ result was not reflected in ‘writeconfig’ output. Later I found that QMP’s ‘object-add’ has the same issue. Anyway, I don’t include ‘qemu_opts’ patches here because Markus mentioned (here http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03476.html) that this functionality is going to be reworked in some future and such patches might collide with the rework process.

Markus, could you please post if you have an update on this topic? Current ‘master’ branch (9993c82dc2f5ce58b41d708b765e1a717ad4281d) still has the issue.

Also, Markus mentioned that once configuration was changed during live migration -- it might be an issue because ‘writeconfig’ data became outdated (and might be make sense to think about to embed this data into migration stream itself). In the same time David said that this is another problem which is unrelated to this patch series. What is your current opinion on this topic? Can we consider these patches to be included into ‘master’ taking into account that not all configuration is dumped by ‘writeconfig’ (‘object_add’ problem), but this can be fixed later?

Best regards,
Vadim

[Qemu-devel] [PATCH 1/4] qmp: introduce 'writeconfig' command

[Vadim Galitsyn]

Add support for `writeconfig' command for QMP monitor.
This is a simple way to keep track of current state of VM
after series of hotplugs and/or hotunplugs of different devices.

Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: qemu-devel@nongnu.org
---
 qapi-schema.json | 18 ++++++++++++++++++
 qmp.c            | 21 +++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index a9dd043f65..083f8f3258 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3200,3 +3200,21 @@
 # Since: 2.11
 ##
 { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
+
+##
+# @writeconfig:
+#
+# Dump current configuration into specified file
+#
+# @file: pathname of a file to store current configuration
+#
+# Since: 2.11
+#
+# Example:
+#
+# -> { "execute": "writeconfig",
+#      "arguments": { "file": "/tmp/qemu.conf" } }
+# <- { "return": {} }
+#
+##
+{ 'command': 'writeconfig', 'data': {'file': 'str'} }
diff --git a/qmp.c b/qmp.c
index e8c303116a..fcb911cabb 100644
--- a/qmp.c
+++ b/qmp.c
@@ -722,3 +722,24 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
 
     return mem_info;
 }
+
+void qmp_writeconfig(const char *file, Error **errp)
+{
+    int fd;
+    FILE *fp;
+
+    fd = qemu_open(file,  O_WRONLY | O_CREAT | O_TRUNC, 0600);
+    if (fd != -1) {
+
+        fp = fdopen(fd, "wb");
+        if (fp) {
+            qemu_config_write(fp);
+            fclose(fp);
+            return;
+        }
+
+        qemu_close(fd);
+    }
+
+    error_setg(errp, "cannot write configuration file: '%s'", file);
+}
-- 
2.13.1.394.g41dd433

Re: [Qemu-devel] [PATCH 1/4] qmp: introduce 'writeconfig' command

[Eduardo Otubo]

On Mon, Oct 23, 2017 at 05:13:07PM +0200, Vadim Galitsyn wrote:
> Add support for `writeconfig' command for QMP monitor.
> This is a simple way to keep track of current state of VM
> after series of hotplugs and/or hotunplugs of different devices.
> 
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  qapi-schema.json | 18 ++++++++++++++++++
>  qmp.c            | 21 +++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a9dd043f65..083f8f3258 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3200,3 +3200,21 @@
>  # Since: 2.11
>  ##
>  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} }
> +
> +##
> +# @writeconfig:
> +#
> +# Dump current configuration into specified file
> +#
> +# @file: pathname of a file to store current configuration
> +#
> +# Since: 2.11
> +#
> +# Example:
> +#
> +# -> { "execute": "writeconfig",
> +#      "arguments": { "file": "/tmp/qemu.conf" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'writeconfig', 'data': {'file': 'str'} }
> diff --git a/qmp.c b/qmp.c
> index e8c303116a..fcb911cabb 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -722,3 +722,24 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)
>  
>      return mem_info;
>  }
> +
> +void qmp_writeconfig(const char *file, Error **errp)
> +{
> +    int fd;
> +    FILE *fp;
> +
> +    fd = qemu_open(file,  O_WRONLY | O_CREAT | O_TRUNC, 0600);
> +    if (fd != -1) {
> +
> +        fp = fdopen(fd, "wb");
> +        if (fp) {
> +            qemu_config_write(fp);
> +            fclose(fp);
> +            return;
> +        }
> +
> +        qemu_close(fd);
> +    }
> +
> +    error_setg(errp, "cannot write configuration file: '%s'", file);
> +}
> -- 
> 2.13.1.394.g41dd433
> 
> 

Reviewed-by: Eduardo Otubo <otubo@redhat.com>

-- 
Eduardo Otubo

Re: [Qemu-devel] [PATCH 1/4] qmp: introduce 'writeconfig' command

[Marc-André Lureau]

Hi

On Mon, Oct 23, 2017 at 5:13 PM, Vadim Galitsyn <
vadim.galitsyn@profitbricks.com> wrote:

> Add support for `writeconfig' command for QMP monitor.
> This is a simple way to keep track of current state of VM
> after series of hotplugs and/or hotunplugs of different devices.
>
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  qapi-schema.json | 18 ++++++++++++++++++
>  qmp.c            | 21 +++++++++++++++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a9dd043f65..083f8f3258 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3200,3 +3200,21 @@
>  # Since: 2.11
>  ##
>  { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'}
> }
> +
> +##
> +# @writeconfig:
> +#
> +# Dump current configuration into specified file
> +#
> +# @file: pathname of a file to store current configuration
> +#
> +# Since: 2.11
> +#
> +# Example:
> +#
> +# -> { "execute": "writeconfig",
> +#      "arguments": { "file": "/tmp/qemu.conf" } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'writeconfig', 'data': {'file': 'str'} }
> diff --git a/qmp.c b/qmp.c
> index e8c303116a..fcb911cabb 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -722,3 +722,24 @@ MemoryInfo *qmp_query_memory_size_summary(Error
> **errp)
>
>      return mem_info;
>  }
> +
> +void qmp_writeconfig(const char *file, Error **errp)
> +{
> +    int fd;
> +    FILE *fp;
> +
> +    fd = qemu_open(file,  O_WRONLY | O_CREAT | O_TRUNC, 0600);
> +    if (fd != -1) {
> +
> +        fp = fdopen(fd, "wb");
> +        if (fp) {
> +            qemu_config_write(fp);
> +            fclose(fp);
>

If you use /dev/fdset, this will close the underlying file, but not remove
the file from the fdset.

Dup it or remove it with monitor_fdset_dup_fd_remove() etc?

+            return;
> +        }
> +
> +        qemu_close(fd);
>

But this will close & remove it from fdset (when the operation failed)

Whatever we do, I think it make sense to document the behaviour.



> +    }
> +
> +    error_setg(errp, "cannot write configuration file: '%s'", file);
> +}
> --
> 2.13.1.394.g41dd433
>
>
>


-- 
Marc-André Lureau

[Qemu-devel] [PATCH 2/4] hmp: introduce 'writeconfig' command

[Vadim Galitsyn]

Add 'writeconfig' command for HMP monitor. This command is a
sibling of QMP command 'writeconfig'.

This is a simple way to keep track of current state of VM
after series of hotplugs and/or hotunplugs of different devices.

Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-devel@nongnu.org
---
 hmp.h           |  1 +
 hmp.c           |  9 +++++++++
 hmp-commands.hx | 17 +++++++++++++++++
 3 files changed, 27 insertions(+)

diff --git a/hmp.h b/hmp.h
index 3605003e4c..2730b7f7b9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -146,5 +146,6 @@ void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
 void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
 void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
+void hmp_writeconfig(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/hmp.c b/hmp.c
index ec61329ebb..20af8ab870 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2901,3 +2901,12 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
     }
     hmp_handle_error(mon, &err);
 }
+
+void hmp_writeconfig(Monitor *mon, const QDict *qdict)
+{
+     const char *file = qdict_get_str(qdict, "file");
+     Error *err = NULL;
+
+     qmp_writeconfig(file, &err);
+     hmp_handle_error(mon, &err);
+}
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1941e19932..98a3dc4b66 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1858,6 +1858,23 @@ ETEXI
         .sub_table  = info_cmds,
     },
 
+    {
+        .name       = "writeconfig",
+        .args_type  = "file:F",
+        .params     = "file",
+        .help       = "dump current configuration into specified file",
+        .cmd        = hmp_writeconfig,
+    },
+
+STEXI
+@item writeconfig @var{file}
+@findex writeconfig
+Dump current configuration into specified @var{file}.
+@example
+(qemu) writeconfig /tmp/qemu.conf
+@end example
+ETEXI
+
 STEXI
 @end table
 ETEXI
-- 
2.13.1.394.g41dd433

Re: [Qemu-devel] [PATCH 2/4] hmp: introduce 'writeconfig' command

[Dr. David Alan Gilbert]

* Vadim Galitsyn (vadim.galitsyn@profitbricks.com) wrote:
> Add 'writeconfig' command for HMP monitor. This command is a
> sibling of QMP command 'writeconfig'.
> 
> This is a simple way to keep track of current state of VM
> after series of hotplugs and/or hotunplugs of different devices.
> 
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: qemu-devel@nongnu.org

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hmp.h           |  1 +
>  hmp.c           |  9 +++++++++
>  hmp-commands.hx | 17 +++++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/hmp.h b/hmp.h
> index 3605003e4c..2730b7f7b9 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -146,5 +146,6 @@ void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
>  void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
>  void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
> +void hmp_writeconfig(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/hmp.c b/hmp.c
> index ec61329ebb..20af8ab870 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2901,3 +2901,12 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
>      }
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_writeconfig(Monitor *mon, const QDict *qdict)
> +{
> +     const char *file = qdict_get_str(qdict, "file");
> +     Error *err = NULL;
> +
> +     qmp_writeconfig(file, &err);
> +     hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 1941e19932..98a3dc4b66 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1858,6 +1858,23 @@ ETEXI
>          .sub_table  = info_cmds,
>      },
>  
> +    {
> +        .name       = "writeconfig",
> +        .args_type  = "file:F",
> +        .params     = "file",
> +        .help       = "dump current configuration into specified file",
> +        .cmd        = hmp_writeconfig,
> +    },
> +
> +STEXI
> +@item writeconfig @var{file}
> +@findex writeconfig
> +Dump current configuration into specified @var{file}.
> +@example
> +(qemu) writeconfig /tmp/qemu.conf
> +@end example
> +ETEXI
> +
>  STEXI
>  @end table
>  ETEXI
> -- 
> 2.13.1.394.g41dd433
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/4] hmp: introduce 'writeconfig' command

[Eduardo Otubo]

On Mon, Oct 23, 2017 at 05:13:08PM +0200, Vadim Galitsyn wrote:
> Add 'writeconfig' command for HMP monitor. This command is a
> sibling of QMP command 'writeconfig'.
> 
> This is a simple way to keep track of current state of VM
> after series of hotplugs and/or hotunplugs of different devices.
> 
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  hmp.h           |  1 +
>  hmp.c           |  9 +++++++++
>  hmp-commands.hx | 17 +++++++++++++++++
>  3 files changed, 27 insertions(+)
> 
> diff --git a/hmp.h b/hmp.h
> index 3605003e4c..2730b7f7b9 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -146,5 +146,6 @@ void hmp_info_ramblock(Monitor *mon, const QDict *qdict);
>  void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict);
>  void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict);
>  void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict);
> +void hmp_writeconfig(Monitor *mon, const QDict *qdict);
>  
>  #endif
> diff --git a/hmp.c b/hmp.c
> index ec61329ebb..20af8ab870 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -2901,3 +2901,12 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
>      }
>      hmp_handle_error(mon, &err);
>  }
> +
> +void hmp_writeconfig(Monitor *mon, const QDict *qdict)
> +{
> +     const char *file = qdict_get_str(qdict, "file");
> +     Error *err = NULL;
> +
> +     qmp_writeconfig(file, &err);
> +     hmp_handle_error(mon, &err);
> +}
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 1941e19932..98a3dc4b66 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1858,6 +1858,23 @@ ETEXI
>          .sub_table  = info_cmds,
>      },
>  
> +    {
> +        .name       = "writeconfig",
> +        .args_type  = "file:F",
> +        .params     = "file",
> +        .help       = "dump current configuration into specified file",
> +        .cmd        = hmp_writeconfig,
> +    },
> +
> +STEXI
> +@item writeconfig @var{file}
> +@findex writeconfig
> +Dump current configuration into specified @var{file}.
> +@example
> +(qemu) writeconfig /tmp/qemu.conf
> +@end example
> +ETEXI
> +
>  STEXI
>  @end table
>  ETEXI
> -- 
> 2.13.1.394.g41dd433
> 
> 
Reviewed-by: Eduardo Otubo <otubo@redhat.com>

-- 
Eduardo Otubo

[Qemu-devel] [PATCH 3/4] tests: test-hmp: extend with 'writeconfig' command

[Vadim Galitsyn]

Extend list of test cases with 'writeconfig' command.

Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-devel@nongnu.org
---
 tests/test-hmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index 5677fbf775..8e21eee61c 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -68,6 +68,7 @@ static const char *hmp_cmds[] = {
     "sum 0 512",
     "x /8i 0x100",
     "xp /16x 0",
+    "writeconfig /dev/null",
     NULL
 };
 
-- 
2.13.1.394.g41dd433

Re: [Qemu-devel] [PATCH 3/4] tests: test-hmp: extend with 'writeconfig' command

[Eduardo Otubo]

On Mon, Oct 23, 2017 at 05:13:09PM +0200, Vadim Galitsyn wrote:
> Extend list of test cases with 'writeconfig' command.
> 
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  tests/test-hmp.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> index 5677fbf775..8e21eee61c 100644
> --- a/tests/test-hmp.c
> +++ b/tests/test-hmp.c
> @@ -68,6 +68,7 @@ static const char *hmp_cmds[] = {
>      "sum 0 512",
>      "x /8i 0x100",
>      "xp /16x 0",
> +    "writeconfig /dev/null",
>      NULL
>  };
>  
> -- 
> 2.13.1.394.g41dd433
> 
> 
Reviewed-by: Eduardo Otubo <otubo@redhat.com>

-- 
Eduardo Otubo

[Qemu-devel] [PATCH 4/4] tests: test-hmp: print command execution result

[Vadim Galitsyn]

Provide HMP monitor command execution result as it would be seen
by user who established an HMP monitor session.

Currently many commands may silently fail without any sign of that.
This patch let this info to be printed once test is running in
verbose mode.

For the future it might be useful to fail the test if command has
failed, however it would require a bit of rework inside test
engine itself.

A simple example of silent failure without reporting it would to
add some non-existent HMP command into 'hmp_cmds' list. In this case
test will report it successfully passed without error.

Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: qemu-devel@nongnu.org
---
 tests/test-hmp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tests/test-hmp.c b/tests/test-hmp.c
index 8e21eee61c..1fd8e44eff 100644
--- a/tests/test-hmp.c
+++ b/tests/test-hmp.c
@@ -79,10 +79,13 @@ static void test_commands(void)
     int i;
 
     for (i = 0; hmp_cmds[i] != NULL; i++) {
+        response = hmp("%s", hmp_cmds[i]);
         if (verbose) {
-            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
+            fprintf(stderr,
+                    "\texecute HMP command: %s\n"
+                    "\tresult             : %s\n",
+                    hmp_cmds[i], response);
         }
-        response = hmp("%s", hmp_cmds[i]);
         g_free(response);
     }
 
-- 
2.13.1.394.g41dd433

Re: [Qemu-devel] [PATCH 4/4] tests: test-hmp: print command execution result

[Eduardo Otubo]

On Mon, Oct 23, 2017 at 05:13:10PM +0200, Vadim Galitsyn wrote:
> Provide HMP monitor command execution result as it would be seen
> by user who established an HMP monitor session.
> 
> Currently many commands may silently fail without any sign of that.
> This patch let this info to be printed once test is running in
> verbose mode.
> 
> For the future it might be useful to fail the test if command has
> failed, however it would require a bit of rework inside test
> engine itself.
> 
> A simple example of silent failure without reporting it would to
> add some non-existent HMP command into 'hmp_cmds' list. In this case
> test will report it successfully passed without error.
> 
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: qemu-devel@nongnu.org
> ---
>  tests/test-hmp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> index 8e21eee61c..1fd8e44eff 100644
> --- a/tests/test-hmp.c
> +++ b/tests/test-hmp.c
> @@ -79,10 +79,13 @@ static void test_commands(void)
>      int i;
>  
>      for (i = 0; hmp_cmds[i] != NULL; i++) {
> +        response = hmp("%s", hmp_cmds[i]);
>          if (verbose) {
> -            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> +            fprintf(stderr,
> +                    "\texecute HMP command: %s\n"
> +                    "\tresult             : %s\n",
> +                    hmp_cmds[i], response);
>          }
> -        response = hmp("%s", hmp_cmds[i]);
>          g_free(response);
>      }
>  
> -- 
> 2.13.1.394.g41dd433
> 
> 

Reviewed-by: Eduardo Otubo <otubo@redhat.com>

-- 
Eduardo Otubo

Re: [Qemu-devel] [PATCH 4/4] tests: test-hmp: print command execution result

[Dr. David Alan Gilbert]

* Vadim Galitsyn (vadim.galitsyn@profitbricks.com) wrote:
> Provide HMP monitor command execution result as it would be seen
> by user who established an HMP monitor session.
> 
> Currently many commands may silently fail without any sign of that.
> This patch let this info to be printed once test is running in
> verbose mode.
> 
> For the future it might be useful to fail the test if command has
> failed, however it would require a bit of rework inside test
> engine itself.
> 
> A simple example of silent failure without reporting it would to
> add some non-existent HMP command into 'hmp_cmds' list. In this case
> test will report it successfully passed without error.
> 
> Signed-off-by: Vadim Galitsyn <vadim.galitsyn@profitbricks.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: qemu-devel@nongnu.org

This test patch is useful without the rest of the series;

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

Queued for HMP

Although I do also need to go and fix this tests idea of verbose to get
it from GTester rather than it's own env - I'll do that.

For the rest of the set, you need to convince Markus why
it's safe.

Dave


> ---
>  tests/test-hmp.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/test-hmp.c b/tests/test-hmp.c
> index 8e21eee61c..1fd8e44eff 100644
> --- a/tests/test-hmp.c
> +++ b/tests/test-hmp.c
> @@ -79,10 +79,13 @@ static void test_commands(void)
>      int i;
>  
>      for (i = 0; hmp_cmds[i] != NULL; i++) {
> +        response = hmp("%s", hmp_cmds[i]);
>          if (verbose) {
> -            fprintf(stderr, "\t%s\n", hmp_cmds[i]);
> +            fprintf(stderr,
> +                    "\texecute HMP command: %s\n"
> +                    "\tresult             : %s\n",
> +                    hmp_cmds[i], response);
>          }
> -        response = hmp("%s", hmp_cmds[i]);
>          g_free(response);
>      }
>  
> -- 
> 2.13.1.394.g41dd433
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] QMP, HMP: introduce 'writeconfig' command

[Dr. David Alan Gilbert]

* Vadim Galitsyn (vadim.galitsyn@profitbricks.com) wrote:
> Hi Guys,
> 
> This thread is a continuation of discussion started in
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03182.html.
> 
> This series introduces ‘writeconfig’ command support for QMP and HMP monitors. This functionality might be useful for live migration for cases when guest configuration was modified in runtime (for example as a result of hot- plug/unplug operations) and actual Qemu command line no longer reflects setup exposed to guest.
> 
> Original series has ‘qemu_opts’ patch as well (http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03183.html) because HMP’s ‘object_add’ result was not reflected in ‘writeconfig’ output. Later I found that QMP’s ‘object-add’ has the same issue. Anyway, I don’t include ‘qemu_opts’ patches here because Markus mentioned (here http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03476.html) that this functionality is going to be reworked in some future and such patches might collide with the rework process.
> 
> Markus, could you please post if you have an update on this topic? Current ‘master’ branch (9993c82dc2f5ce58b41d708b765e1a717ad4281d) still has the issue.
> 
> Also, Markus mentioned that once configuration was changed during live migration -- it might be an issue because ‘writeconfig’ data became outdated (and might be make sense to think about to embed this data into migration stream itself). In the same time David said that this is another problem which is unrelated to this patch series. What is your current opinion on this topic? Can we consider these patches to be included into ‘master’ taking into account that not all configuration is dumped by ‘writeconfig’ (‘object_add’ problem), but this can be fixed later?

I don't see anything wrong with having a 'writeconfig' command - we
already have the command line equivalent - that is assuming hotplug
etc all cause the values written to be correct.

The data becoming outdated doesn't sound like a big issue to me;
it's the management layer that's doing any hotplugs, it can reissue
another 'writeconfig' command.

Dave

> 
> Best regards,
> Vadim
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] QMP, HMP: introduce 'writeconfig' command

[Markus Armbruster]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Vadim Galitsyn (vadim.galitsyn@profitbricks.com) wrote:
>> Hi Guys,
>> 
>> This thread is a continuation of discussion started in
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03182.html.
>> 
>> This series introduces ‘writeconfig’ command support for QMP and HMP
>> monitors. This functionality might be useful for live migration for
>> cases when guest configuration was modified in runtime (for example
>> as a result of hot- plug/unplug operations) and actual Qemu command
>> line no longer reflects setup exposed to guest.
>> 
>> Original series has ‘qemu_opts’ patch as well
>> (http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03183.html)
>> because HMP’s ‘object_add’ result was not reflected in ‘writeconfig’
>> output. Later I found that QMP’s ‘object-add’ has the same
>> issue. Anyway, I don’t include ‘qemu_opts’ patches here because
>> Markus mentioned (here
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03476.html)
>> that this functionality is going to be reworked in some future and
>> such patches might collide with the rework process.

This is item 1 of my reply.  Item 3 is more important:

   The accuracy of QemuOpts information is doubtful.

   Completeness: only certain kinds of configuration are done with
   QemuOpts.  Incompleteness makes -writeconfig less useful than it
   could be, but it's still useful.  Monitor command writeconfig could
   be similarly useful.

   Correctness: configuration gets stored in QemuOpts when we parse
   KEY=VALUE,... strings.  It can also be constructed and updated
   manually.  At certain points in time, bits from QemuOpts are used to
   actually configure stuff.

   Example: -device creates an entry in the "device" configuration
   group, which is later used to actually create and configure a device
   object.

   My point is: whenever we manipulate the actual objects, we may
   invalidate information stored in QemuOpts.  We can try to keep it in
   sync, and we do at least sometimes.  But this is a game we can only
   lose, except for the period(s) of time where QemuOpts is all there
   is, i.e. before actual objects get created.  Note that -writeconfig
   runs before objects get created, so it's not affected by this issue.

   Out-of-sync QemuOpts is harmless unless something relies on it being
   accurate.  I know we currently rely on QemuOpts IDs to catch
   duplicate IDs for some of the configuration groups.  I doubt there's
   much else.

   If we add your monitor command, out-of-sync QemuOpts goes from
   harmless to serious bug.  In other words, we'd create a new class of
   bugs, with an unknown number of existing instances that are probably
   hard to find and fix.  Probably a perpetual source of new instances,
   too.

   Feels like a show stopper to me.

Message-ID: <87k28qlca9.fsf@dusky.pond.sub.org>

>> Markus, could you please post if you have an update on this topic?
>> Current ‘master’ branch (9993c82dc2f5ce58b41d708b765e1a717ad4281d)
>> still has the issue.

There's my "[RFC PATCH 00/32] Command line QAPIfication"
Message-Id: <20171002152552.27999-1-armbru@redhat.com>
https://lists.nongnu.org/archive/html/qemu-devel/2017-10/msg00209.html

There's also my KVM Forum presentation.  Permanent links to slides and
hopefully video should appear at
http://www.linux-kvm.org/page/KVM_Forum_2017 "soon".  Until then, you
can also get my slides from
https://events.linuxfoundation.org/sites/events/files/slides/armbru-qapi-cmdline_1.pdf

>> Also, Markus mentioned that once configuration was changed during
>> live migration -- it might be an issue because ‘writeconfig’ data
>> became outdated (and might be make sense to think about to embed this
>> data into migration stream itself). In the same time David said that
>> this is another problem which is unrelated to this patch series. What
>> is your current opinion on this topic? Can we consider these patches
>> to be included into ‘master’ taking into account that not all
>> configuration is dumped by ‘writeconfig’ (‘object_add’ problem), but
>> this can be fixed later?
>
> I don't see anything wrong with having a 'writeconfig' command - we
> already have the command line equivalent - that is assuming hotplug
> etc all cause the values written to be correct.

In the current state of things, this assumption is somewhere between
reckless and wrong.  See "this is a game we can only lose" above.

> The data becoming outdated doesn't sound like a big issue to me;
> it's the management layer that's doing any hotplugs, it can reissue
> another 'writeconfig' command.

It can, but how would it use the data?  It already started the
destination QEMU with configuration derived from the first writeconfig,
plus -incoming.  Source configuration may change until we're ready to
pivot from to the destination.  A second writeconfig only tells the
management application what configuration it has to reach on the
destination, but not how it could be done.  Perhaps that helps, perhaps
it doesn't.

All theoretical, because the data cannot be relied on anyway.

Re: [Qemu-devel] QMP, HMP: introduce 'writeconfig' command

[Eric Blake]

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

On 10/23/2017 10:13 AM, Vadim Galitsyn wrote:
> Hi Guys,
> 
> This thread is a continuation of discussion started in
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03182.html.
> 
> This series introduces ‘writeconfig’ command support for QMP and HMP monitors. This functionality might be useful for live migration for cases when guest configuration was modified in runtime (for example as a result of hot- plug/unplug operations) and actual Qemu command line no longer reflects setup exposed to guest.
> 
> Original series has ‘qemu_opts’ patch as well (http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03183.html) because HMP’s ‘object_add’ result was not reflected in ‘writeconfig’ output. Later I found that QMP’s ‘object-add’ has the same issue. Anyway, I don’t include ‘qemu_opts’ patches here because Markus mentioned (here http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg03476.html) that this functionality is going to be reworked in some future and such patches might collide with the rework process.
> 
> Markus, could you please post if you have an update on this topic? Current ‘master’ branch (9993c82dc2f5ce58b41d708b765e1a717ad4281d) still has the issue.
> 
> Also, Markus mentioned that once configuration was changed during live migration -- it might be an issue because ‘writeconfig’ data became outdated (and might be make sense to think about to embed this data into migration stream itself). In the same time David said that this is another problem which is unrelated to this patch series. What is your current opinion on this topic? Can we consider these patches to be included into ‘master’ taking into account that not all configuration is dumped by ‘writeconfig’ (‘object_add’ problem), but this can be fixed later?

I don't think we should expose 'writeconfig' via QMP as long as there is
still the chance of inconsistent data being written.  And I think we
have a lot more issues where existing code abuses QemuOpts in ways that
current configuration does not match the original command line, but
where you cannot easily expose the current configuration in a way that
would be reparsed by the command line into the current state.
Therefore, I'm not sure this series is worthwhile.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]