[Qemu-devel] [PULL 0/9] Removal of deprecated options and improvements for qtests

[Qemu-devel] [PULL 0/9] Removal of deprecated options and improvements for qtests

[Thomas Huth]

 Hi Peter!

The following changes since commit 19b599f7664b2ebfd0f405fb79c14dd241557452:

  Merge remote-tracking branch 'remotes/armbru/tags/pull-error-2018-08-27-v2' into staging (2018-08-27 16:44:20 +0100)

are available in the git repository at:

  https://gitlab.com/huth/qemu.git tags/pull-request-2018-08-31

for you to fetch changes up to ae6bf766048ecaeef90b85c4fb2b4db2aa0c094c:

  tests: add a qmp success-response test (2018-08-31 09:53:10 +0200)

----------------------------------------------------------------
Removal of deprecated options and improvements for the qtests
----------------------------------------------------------------

Eric Blake (1):
      tests/libqos: Utilize newer glib spawn check

Marc-André Lureau (4):
      tests: add qmp_assert_error_class()
      tests: add qmp/object-add-without-props test
      tests: add qmp/qom-set-without-value test
      tests: add a qmp success-response test

Thomas Huth (4):
      Remove the deprecated -balloon option
      Remove the deprecated -nodefconfig option
      Remove the deprecated options -startdate, -localtime and -rtc-td-hack
      net: Remove the deprecated -tftp, -bootp, -redir and -smb options

 docs/interop/live-block-operations.rst  |   4 +-
 docs/virtio-balloon-stats.txt           |   6 +-
 include/net/net.h                       |   3 -
 include/net/slirp.h                     |   4 -
 net/slirp.c                             | 132 +++++++-------------------------
 os-posix.c                              |   8 --
 qemu-deprecated.texi                    |  56 --------------
 qemu-options.hx                         |  36 +--------
 tests/drive_del-test.c                  |   5 +-
 tests/libqos/libqos.c                   |  12 +--
 tests/libqtest.c                        |  11 +++
 tests/libqtest.h                        |   9 +++
 tests/qapi-schema/qapi-schema-test.json |   2 +
 tests/qapi-schema/qapi-schema-test.out  |   2 +
 tests/qmp-cmd-test.c                    |  18 +++++
 tests/qmp-test.c                        |  87 +++++++++------------
 tests/test-qga.c                        |   9 +--
 tests/test-qmp-cmds.c                   |  17 ++++
 vl.c                                    | 132 ++++++--------------------------
 19 files changed, 158 insertions(+), 395 deletions(-)

[Qemu-devel] [PULL 1/9] Remove the deprecated -balloon option

[Thomas Huth]

The "-balloon" option has been replaced by "-device virtio-balloon".
It's been marked as deprecated since two releases, and nobody
complained, so let's remove it now.

Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Acked-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/virtio-balloon-stats.txt |  6 +++---
 qemu-deprecated.texi          |  5 -----
 qemu-options.hx               | 10 ----------
 vl.c                          | 36 ------------------------------------
 4 files changed, 3 insertions(+), 54 deletions(-)

diff --git a/docs/virtio-balloon-stats.txt b/docs/virtio-balloon-stats.txt
index 9985e1d..1732cc8 100644
--- a/docs/virtio-balloon-stats.txt
+++ b/docs/virtio-balloon-stats.txt
@@ -61,9 +61,9 @@ It's also important to note the following:
    respond to the request the timer will never be re-armed, which has
    the same effect as disabling polling
 
-Here are a few examples. QEMU is started with '-balloon virtio', which
-generates '/machine/peripheral-anon/device[1]' as the QOM path for the
-balloon device.
+Here are a few examples. QEMU is started with '-device virtio-balloon',
+which generates '/machine/peripheral-anon/device[1]' as the QOM path for
+the balloon device.
 
 Enable polling with 2 seconds interval:
 
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 1b9c007..98f5062 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -86,11 +86,6 @@ enabled via the ``-machine usb=on'' argument.
 
 The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
 
-@subsection -balloon (since 2.12.0)
-
-The @option{--balloon virtio} argument has been superseded by
-@option{--device virtio-balloon}.
-
 @subsection -fsdev handle (since 2.12.0)
 
 The ``handle'' fsdev backend does not support symlinks and causes the 9p
diff --git a/qemu-options.hx b/qemu-options.hx
index 654ef48..ccf7155 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -454,16 +454,6 @@ modprobe i810_audio clocking=48000
 @end example
 ETEXI
 
-DEF("balloon", HAS_ARG, QEMU_OPTION_balloon,
-    "-balloon virtio[,addr=str]\n"
-    "                enable virtio balloon device (deprecated)\n", QEMU_ARCH_ALL)
-STEXI
-@item -balloon virtio[,addr=@var{addr}]
-@findex -balloon
-Enable virtio balloon device, optionally with PCI address @var{addr}. This
-option is deprecated, use @option{-device virtio-balloon} instead.
-ETEXI
-
 DEF("device", HAS_ARG, QEMU_OPTION_device,
     "-device driver[,prop[=value][,...]]\n"
     "                add device (based on driver)\n"
diff --git a/vl.c b/vl.c
index 5ba06ad..674c42f 100644
--- a/vl.c
+++ b/vl.c
@@ -2127,36 +2127,6 @@ static void parse_display(const char *p)
     }
 }
 
-static int balloon_parse(const char *arg)
-{
-    QemuOpts *opts;
-
-    warn_report("This option is deprecated. "
-                "Use '--device virtio-balloon' to enable the balloon device.");
-
-    if (strcmp(arg, "none") == 0) {
-        return 0;
-    }
-
-    if (!strncmp(arg, "virtio", 6)) {
-        if (arg[6] == ',') {
-            /* have params -> parse them */
-            opts = qemu_opts_parse_noisily(qemu_find_opts("device"), arg + 7,
-                                           false);
-            if (!opts)
-                return  -1;
-        } else {
-            /* create empty opts */
-            opts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
-                                    &error_abort);
-        }
-        qemu_opt_set(opts, "driver", "virtio-balloon", &error_abort);
-        return 0;
-    }
-
-    return -1;
-}
-
 char *qemu_find_file(int type, const char *name)
 {
     int i;
@@ -3660,12 +3630,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_no_hpet:
                 no_hpet = 1;
                 break;
-            case QEMU_OPTION_balloon:
-                if (balloon_parse(optarg) < 0) {
-                    error_report("unknown -balloon argument %s", optarg);
-                    exit(1);
-                }
-                break;
             case QEMU_OPTION_no_reboot:
                 no_reboot = 1;
                 break;
-- 
1.8.3.1

[Qemu-devel] [PULL 2/9] Remove the deprecated -nodefconfig option

[Thomas Huth]

It's the same as -no-user-config and marked as deprecated since three
releases already. Time to remove it now.

Acked-by: Peter Krempa <pkrempa@redhat.com>
Acked-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 docs/interop/live-block-operations.rst | 4 ++--
 qemu-deprecated.texi                   | 4 ----
 qemu-options.hx                        | 4 ++--
 vl.c                                   | 2 --
 4 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/docs/interop/live-block-operations.rst b/docs/interop/live-block-operations.rst
index 734252b..48afdc7 100644
--- a/docs/interop/live-block-operations.rst
+++ b/docs/interop/live-block-operations.rst
@@ -129,7 +129,7 @@ To show some example invocations of command-line, we will use the
 following invocation of QEMU, with a QMP server running over UNIX
 socket::
 
-    $ ./x86_64-softmmu/qemu-system-x86_64 -display none -nodefconfig \
+    $ ./x86_64-softmmu/qemu-system-x86_64 -display none -no-user-config \
         -M q35 -nodefaults -m 512 \
         -blockdev node-name=node-A,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./a.qcow2 \
         -device virtio-blk,drive=node-A,id=virtio0 \
@@ -694,7 +694,7 @@ instance, with the following invocation.  (As noted earlier, for
 simplicity's sake, the destination QEMU is started on the same host, but
 it could be located elsewhere)::
 
-    $ ./x86_64-softmmu/qemu-system-x86_64 -display none -nodefconfig \
+    $ ./x86_64-softmmu/qemu-system-x86_64 -display none -no-user-config \
         -M q35 -nodefaults -m 512 \
         -blockdev node-name=node-TargetDisk,driver=qcow2,file.driver=file,file.node-name=file,file.filename=./target-disk.qcow2 \
         -device virtio-blk,drive=node-TargetDisk,id=virtio0 \
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 98f5062..19c8ae2 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -82,10 +82,6 @@ would automatically enable USB support on the machine type.
 If using the new syntax, USB support must be explicitly
 enabled via the ``-machine usb=on'' argument.
 
-@subsection -nodefconfig (since 2.11.0)
-
-The ``-nodefconfig`` argument is a synonym for ``-no-user-config``.
-
 @subsection -fsdev handle (since 2.12.0)
 
 The ``handle'' fsdev backend does not support symlinks and causes the 9p
diff --git a/qemu-options.hx b/qemu-options.hx
index ccf7155..d9be20b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3833,8 +3833,7 @@ Write device configuration to @var{file}. The @var{file} can be either filename
 command line and device configuration into file or dash @code{-}) character to print the
 output to stdout. This can be later used as input file for @code{-readconfig} option.
 ETEXI
-HXCOMM Deprecated, same as -no-user-config
-DEF("nodefconfig", 0, QEMU_OPTION_nodefconfig, "", QEMU_ARCH_ALL)
+
 DEF("no-user-config", 0, QEMU_OPTION_nouserconfig,
     "-no-user-config\n"
     "                do not load default user-provided config files at startup\n",
@@ -3845,6 +3844,7 @@ STEXI
 The @code{-no-user-config} option makes QEMU not load any of the user-provided
 config files on @var{sysconfdir}.
 ETEXI
+
 DEF("trace", HAS_ARG, QEMU_OPTION_trace,
     "-trace [[enable=]<pattern>][,events=<file>][,file=<file>]\n"
     "                specify tracing options\n",
diff --git a/vl.c b/vl.c
index 674c42f..386c71d 100644
--- a/vl.c
+++ b/vl.c
@@ -2999,7 +2999,6 @@ int main(int argc, char **argv, char **envp)
 
             popt = lookup_opt(argc, argv, &optarg, &optind);
             switch (popt->index) {
-            case QEMU_OPTION_nodefconfig:
             case QEMU_OPTION_nouserconfig:
                 userconfig = false;
                 break;
@@ -3927,7 +3926,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_enable_sync_profile:
                 qsp_enable();
                 break;
-            case QEMU_OPTION_nodefconfig:
             case QEMU_OPTION_nouserconfig:
                 /* Nothing to be parsed here. Especially, do not error out below. */
                 break;
-- 
1.8.3.1

[Qemu-devel] [PULL 3/9] Remove the deprecated options -startdate, -localtime and -rtc-td-hack

[Thomas Huth]

Deprecated since two releases, nobody complained, thus it's time to
remove them now.

Acked-by: Peter Krempa <pkrempa@redhat.com>
Acked-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qemu-deprecated.texi | 13 ---------
 qemu-options.hx      |  7 -----
 vl.c                 | 76 +++++++++++++++-------------------------------------
 3 files changed, 22 insertions(+), 74 deletions(-)

diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 19c8ae2..ca52e83 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -96,19 +96,6 @@ The @code{--no-frame} argument works with SDL 1.2 only. The other user
 interfaces never implemented this in the first place. So this will be
 removed together with SDL 1.2 support.
 
-@subsection -rtc-td-hack (since 2.12.0)
-
-The @code{-rtc-td-hack} option has been replaced by
-@code{-rtc driftfix=slew}.
-
-@subsection -localtime (since 2.12.0)
-
-The @code{-localtime} option has been replaced by @code{-rtc base=localtime}.
-
-@subsection -startdate (since 2.12.0)
-
-The @code{-startdate} option has been replaced by @code{-rtc base=@var{date}}.
-
 @subsection -virtioconsole (since 3.0.0)
 
 Option @option{-virtioconsole} has been replaced by
diff --git a/qemu-options.hx b/qemu-options.hx
index d9be20b..7ca539a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1711,9 +1711,6 @@ Windows 2000 is installed, you no longer need this option (this option
 slows down the IDE transfers).
 ETEXI
 
-HXCOMM Deprecated by -rtc
-DEF("rtc-td-hack", 0, QEMU_OPTION_rtc_td_hack, "", QEMU_ARCH_I386)
-
 DEF("no-fd-bootchk", 0, QEMU_OPTION_no_fd_bootchk,
     "-no-fd-bootchk  disable boot signature checking for floppy disks\n",
     QEMU_ARCH_I386)
@@ -3471,10 +3468,6 @@ ETEXI
 HXCOMM Silently ignored for compatibility
 DEF("clock", HAS_ARG, QEMU_OPTION_clock, "", QEMU_ARCH_ALL)
 
-HXCOMM Options deprecated by -rtc
-DEF("localtime", 0, QEMU_OPTION_localtime, "", QEMU_ARCH_ALL)
-DEF("startdate", HAS_ARG, QEMU_OPTION_startdate, "", QEMU_ARCH_ALL)
-
 DEF("rtc", HAS_ARG, QEMU_OPTION_rtc, \
     "-rtc [base=utc|localtime|date][,clock=host|rt|vm][,driftfix=none|slew]\n" \
     "                set the RTC base and clock, enable drift fix for clock ticks (x86 only)\n",
diff --git a/vl.c b/vl.c
index 386c71d..cfeee0d 100644
--- a/vl.c
+++ b/vl.c
@@ -823,44 +823,33 @@ int qemu_timedate_diff(struct tm *tm)
     return seconds - qemu_time();
 }
 
-static void configure_rtc_date_offset(const char *startdate, int legacy)
+static void configure_rtc_date_offset(const char *startdate)
 {
     time_t rtc_start_date;
     struct tm tm;
 
-    if (!strcmp(startdate, "now") && legacy) {
-        rtc_date_offset = -1;
+    if (sscanf(startdate, "%d-%d-%dT%d:%d:%d", &tm.tm_year, &tm.tm_mon,
+               &tm.tm_mday, &tm.tm_hour, &tm.tm_min, &tm.tm_sec) == 6) {
+        /* OK */
+    } else if (sscanf(startdate, "%d-%d-%d",
+                      &tm.tm_year, &tm.tm_mon, &tm.tm_mday) == 3) {
+        tm.tm_hour = 0;
+        tm.tm_min = 0;
+        tm.tm_sec = 0;
     } else {
-        if (sscanf(startdate, "%d-%d-%dT%d:%d:%d",
-                   &tm.tm_year,
-                   &tm.tm_mon,
-                   &tm.tm_mday,
-                   &tm.tm_hour,
-                   &tm.tm_min,
-                   &tm.tm_sec) == 6) {
-            /* OK */
-        } else if (sscanf(startdate, "%d-%d-%d",
-                          &tm.tm_year,
-                          &tm.tm_mon,
-                          &tm.tm_mday) == 3) {
-            tm.tm_hour = 0;
-            tm.tm_min = 0;
-            tm.tm_sec = 0;
-        } else {
-            goto date_fail;
-        }
-        tm.tm_year -= 1900;
-        tm.tm_mon--;
-        rtc_start_date = mktimegm(&tm);
-        if (rtc_start_date == -1) {
-        date_fail:
-            error_report("invalid date format");
-            error_printf("valid formats: "
-                         "'2006-06-17T16:01:21' or '2006-06-17'\n");
-            exit(1);
-        }
-        rtc_date_offset = qemu_time() - rtc_start_date;
+        goto date_fail;
+    }
+    tm.tm_year -= 1900;
+    tm.tm_mon--;
+    rtc_start_date = mktimegm(&tm);
+    if (rtc_start_date == -1) {
+    date_fail:
+        error_report("invalid date format");
+        error_printf("valid formats: "
+                     "'2006-06-17T16:01:21' or '2006-06-17'\n");
+        exit(1);
     }
+    rtc_date_offset = qemu_time() - rtc_start_date;
 }
 
 static void configure_rtc(QemuOpts *opts)
@@ -878,7 +867,7 @@ static void configure_rtc(QemuOpts *opts)
                       "-rtc base=localtime");
             replay_add_blocker(blocker);
         } else {
-            configure_rtc_date_offset(value, 0);
+            configure_rtc_date_offset(value);
         }
     }
     value = qemu_opt_get(opts, "clock");
@@ -3269,11 +3258,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_k:
                 keyboard_layout = optarg;
                 break;
-            case QEMU_OPTION_localtime:
-                rtc_utc = 0;
-                warn_report("This option is deprecated, "
-                            "use '-rtc base=localtime' instead.");
-                break;
             case QEMU_OPTION_vga:
                 vga_model = optarg;
                 default_vga = 0;
@@ -3526,18 +3510,6 @@ int main(int argc, char **argv, char **envp)
             case QEMU_OPTION_win2k_hack:
                 win2k_install_hack = 1;
                 break;
-            case QEMU_OPTION_rtc_td_hack: {
-                static GlobalProperty slew_lost_ticks = {
-                    .driver   = "mc146818rtc",
-                    .property = "lost_tick_policy",
-                    .value    = "slew",
-                };
-
-                qdev_prop_register_global(&slew_lost_ticks);
-                warn_report("This option is deprecated, "
-                            "use '-rtc driftfix=slew' instead.");
-                break;
-            }
             case QEMU_OPTION_acpitable:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("acpi"),
                                                optarg, true);
@@ -3723,10 +3695,6 @@ int main(int argc, char **argv, char **envp)
                  */
                 warn_report("This option is ignored and will be removed soon");
                 break;
-            case QEMU_OPTION_startdate:
-                warn_report("This option is deprecated, use '-rtc base=' instead.");
-                configure_rtc_date_offset(optarg, 1);
-                break;
             case QEMU_OPTION_rtc:
                 opts = qemu_opts_parse_noisily(qemu_find_opts("rtc"), optarg,
                                                false);
-- 
1.8.3.1

[Qemu-devel] [PULL 4/9] net: Remove the deprecated -tftp, -bootp, -redir and -smb options

[Thomas Huth]

These options likely do not work as expected as soon as the user
tries to use more than one network interface at once. The parameters
have been marked as deprecated since QEMU v2.6, so users had plenty
of time to move their scripts to the new syntax. Time to remove the
old parameters now.

Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Acked-by: Peter Krempa <pkrempa@redhat.com>
Acked-by: Ján Tomko <jtomko@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 include/net/net.h    |   3 --
 include/net/slirp.h  |   4 --
 net/slirp.c          | 132 +++++++++++----------------------------------------
 os-posix.c           |   8 ----
 qemu-deprecated.texi |  34 -------------
 qemu-options.hx      |  15 ------
 vl.c                 |  18 -------
 7 files changed, 29 insertions(+), 185 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 1425960..7936d53 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -201,9 +201,6 @@ extern NICInfo nd_table[MAX_NICS];
 extern const char *host_net_devices[];
 
 /* from net.c */
-extern const char *legacy_tftp_prefix;
-extern const char *legacy_bootp_filename;
-
 int net_client_parse(QemuOptsList *opts_list, const char *str);
 int net_init_clients(Error **errp);
 void net_check_clients(void);
diff --git a/include/net/slirp.h b/include/net/slirp.h
index 4d63d74..bad3e1e 100644
--- a/include/net/slirp.h
+++ b/include/net/slirp.h
@@ -30,10 +30,6 @@
 void hmp_hostfwd_add(Monitor *mon, const QDict *qdict);
 void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict);
 
-int net_slirp_redir(const char *redir_str);
-
-int net_slirp_smb(const char *exported_dir);
-
 void hmp_info_usernet(Monitor *mon, const QDict *qdict);
 
 #endif
diff --git a/net/slirp.c b/net/slirp.c
index 1e14318..c18060f 100644
--- a/net/slirp.c
+++ b/net/slirp.c
@@ -67,13 +67,11 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
 /* slirp network adapter */
 
 #define SLIRP_CFG_HOSTFWD 1
-#define SLIRP_CFG_LEGACY  2
 
 struct slirp_config_str {
     struct slirp_config_str *next;
     int flags;
     char str[1024];
-    int legacy_format;
 };
 
 typedef struct SlirpState {
@@ -87,19 +85,13 @@ typedef struct SlirpState {
 } SlirpState;
 
 static struct slirp_config_str *slirp_configs;
-const char *legacy_tftp_prefix;
-const char *legacy_bootp_filename;
 static QTAILQ_HEAD(slirp_stacks, SlirpState) slirp_stacks =
     QTAILQ_HEAD_INITIALIZER(slirp_stacks);
 
-static int slirp_hostfwd(SlirpState *s, const char *redir_str,
-                         int legacy_format, Error **errp);
-static int slirp_guestfwd(SlirpState *s, const char *config_str,
-                          int legacy_format, Error **errp);
+static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp);
+static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp);
 
 #ifndef _WIN32
-static const char *legacy_smb_export;
-
 static int slirp_smb(SlirpState *s, const char *exported_dir,
                      struct in_addr vserver_addr, Error **errp);
 static void slirp_smb_cleanup(SlirpState *s);
@@ -196,13 +188,6 @@ static int net_slirp_init(NetClientState *peer, const char *model,
         return -1;
     }
 
-    if (!tftp_export) {
-        tftp_export = legacy_tftp_prefix;
-    }
-    if (!bootfile) {
-        bootfile = legacy_bootp_filename;
-    }
-
     if (vnetwork) {
         if (get_str_sep(buf, sizeof(buf), &vnetwork, '/') < 0) {
             if (!inet_aton(vnetwork, &net)) {
@@ -382,21 +367,16 @@ static int net_slirp_init(NetClientState *peer, const char *model,
 
     for (config = slirp_configs; config; config = config->next) {
         if (config->flags & SLIRP_CFG_HOSTFWD) {
-            if (slirp_hostfwd(s, config->str,
-                              config->flags & SLIRP_CFG_LEGACY, errp) < 0) {
+            if (slirp_hostfwd(s, config->str, errp) < 0) {
                 goto error;
             }
         } else {
-            if (slirp_guestfwd(s, config->str,
-                               config->flags & SLIRP_CFG_LEGACY, errp) < 0) {
+            if (slirp_guestfwd(s, config->str, errp) < 0) {
                 goto error;
             }
         }
     }
 #ifndef _WIN32
-    if (!smb_export) {
-        smb_export = legacy_smb_export;
-    }
     if (smb_export) {
         if (slirp_smb(s, smb_export, smbsrv, errp) < 0) {
             goto error;
@@ -506,8 +486,7 @@ void hmp_hostfwd_remove(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "invalid format\n");
 }
 
-static int slirp_hostfwd(SlirpState *s, const char *redir_str,
-                         int legacy_format, Error **errp)
+static int slirp_hostfwd(SlirpState *s, const char *redir_str, Error **errp)
 {
     struct in_addr host_addr = { .s_addr = INADDR_ANY };
     struct in_addr guest_addr = { .s_addr = 0 };
@@ -532,18 +511,16 @@ static int slirp_hostfwd(SlirpState *s, const char *redir_str,
         goto fail_syntax;
     }
 
-    if (!legacy_format) {
-        if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-            fail_reason = "Missing : separator";
-            goto fail_syntax;
-        }
-        if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
-            fail_reason = "Bad host address";
-            goto fail_syntax;
-        }
+    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        fail_reason = "Missing : separator";
+        goto fail_syntax;
+    }
+    if (buf[0] != '\0' && !inet_aton(buf, &host_addr)) {
+        fail_reason = "Bad host address";
+        goto fail_syntax;
     }
 
-    if (get_str_sep(buf, sizeof(buf), &p, legacy_format ? ':' : '-') < 0) {
+    if (get_str_sep(buf, sizeof(buf), &p, '-') < 0) {
         fail_reason = "Bad host port separator";
         goto fail_syntax;
     }
@@ -602,35 +579,13 @@ void hmp_hostfwd_add(Monitor *mon, const QDict *qdict)
     }
     if (s) {
         Error *err = NULL;
-        if (slirp_hostfwd(s, redir_str, 0, &err) < 0) {
+        if (slirp_hostfwd(s, redir_str, &err) < 0) {
             error_report_err(err);
         }
     }
 
 }
 
-int net_slirp_redir(const char *redir_str)
-{
-    struct slirp_config_str *config;
-    Error *err = NULL;
-    int res;
-
-    if (QTAILQ_EMPTY(&slirp_stacks)) {
-        config = g_malloc(sizeof(*config));
-        pstrcpy(config->str, sizeof(config->str), redir_str);
-        config->flags = SLIRP_CFG_HOSTFWD | SLIRP_CFG_LEGACY;
-        config->next = slirp_configs;
-        slirp_configs = config;
-        return 0;
-    }
-
-    res = slirp_hostfwd(QTAILQ_FIRST(&slirp_stacks), redir_str, 1, &err);
-    if (res < 0) {
-        error_report_err(err);
-    }
-    return res;
-}
-
 #ifndef _WIN32
 
 /* automatic user mode samba server configuration */
@@ -746,28 +701,6 @@ static int slirp_smb(SlirpState* s, const char *exported_dir,
     return 0;
 }
 
-/* automatic user mode samba server configuration (legacy interface) */
-int net_slirp_smb(const char *exported_dir)
-{
-    struct in_addr vserver_addr = { .s_addr = 0 };
-
-    if (legacy_smb_export) {
-        fprintf(stderr, "-smb given twice\n");
-        return -1;
-    }
-    legacy_smb_export = exported_dir;
-    if (!QTAILQ_EMPTY(&slirp_stacks)) {
-        Error *err = NULL;
-        int res = slirp_smb(QTAILQ_FIRST(&slirp_stacks), exported_dir,
-                            vserver_addr, &err);
-        if (res < 0) {
-            error_report_err(err);
-        }
-        return res;
-    }
-    return 0;
-}
-
 #endif /* !defined(_WIN32) */
 
 struct GuestFwd {
@@ -789,8 +722,7 @@ static void guestfwd_read(void *opaque, const uint8_t *buf, int size)
     slirp_socket_recv(fwd->slirp, fwd->server, fwd->port, buf, size);
 }
 
-static int slirp_guestfwd(SlirpState *s, const char *config_str,
-                          int legacy_format, Error **errp)
+static int slirp_guestfwd(SlirpState *s, const char *config_str, Error **errp)
 {
     struct in_addr server = { .s_addr = 0 };
     struct GuestFwd *fwd;
@@ -800,26 +732,20 @@ static int slirp_guestfwd(SlirpState *s, const char *config_str,
     int port;
 
     p = config_str;
-    if (legacy_format) {
-        if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-            goto fail_syntax;
-        }
-    } else {
-        if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-            goto fail_syntax;
-        }
-        if (strcmp(buf, "tcp") && buf[0] != '\0') {
-            goto fail_syntax;
-        }
-        if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
-            goto fail_syntax;
-        }
-        if (buf[0] != '\0' && !inet_aton(buf, &server)) {
-            goto fail_syntax;
-        }
-        if (get_str_sep(buf, sizeof(buf), &p, '-') < 0) {
-            goto fail_syntax;
-        }
+    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        goto fail_syntax;
+    }
+    if (strcmp(buf, "tcp") && buf[0] != '\0') {
+        goto fail_syntax;
+    }
+    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        goto fail_syntax;
+    }
+    if (buf[0] != '\0' && !inet_aton(buf, &server)) {
+        goto fail_syntax;
+    }
+    if (get_str_sep(buf, sizeof(buf), &p, '-') < 0) {
+        goto fail_syntax;
     }
     port = strtol(buf, &end, 10);
     if (*end != '\0' || port < 1 || port > 65535) {
diff --git a/os-posix.c b/os-posix.c
index 9ce6f74..8f39447 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -168,14 +168,6 @@ static bool os_parse_runas_uid_gid(const char *optarg)
 int os_parse_cmd_args(int index, const char *optarg)
 {
     switch (index) {
-#ifdef CONFIG_SLIRP
-    case QEMU_OPTION_smb:
-        error_report("The -smb option is deprecated. "
-                     "Please use '-netdev user,smb=...' instead.");
-        if (net_slirp_smb(optarg) < 0)
-            exit(1);
-        break;
-#endif
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
         if (user_pwd) {
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index ca52e83..8a2e399 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -40,40 +40,6 @@ which is the default.
 The ``-no-kvm'' argument is now a synonym for setting
 ``-machine accel=tcg''.
 
-@subsection -tftp (since 2.6.0)
-
-The ``-tftp /some/dir'' argument is replaced by either
-``-netdev user,id=x,tftp=/some/dir '' (for pluggable NICs, accompanied
-with ``-device ...,netdev=x''), or ``-nic user,tftp=/some/dir''
-(for embedded NICs). The new syntax allows different settings to be
-provided per NIC.
-
-@subsection -bootp (since 2.6.0)
-
-The ``-bootp /some/file'' argument is replaced by either
-``-netdev user,id=x,bootp=/some/file '' (for pluggable NICs, accompanied
-with ``-device ...,netdev=x''), or ``-nic user,bootp=/some/file''
-(for embedded NICs). The new syntax allows different settings to be
-provided per NIC.
-
-@subsection -redir (since 2.6.0)
-
-The ``-redir [tcp|udp]:hostport:[guestaddr]:guestport'' argument is
-replaced by either
-``-netdev user,id=x,hostfwd=[tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport''
-(for pluggable NICs, accompanied with ``-device ...,netdev=x'') or
-``-nic user,hostfwd=[tcp|udp]:[hostaddr]:hostport-[guestaddr]:guestport''
-(for embedded NICs). The new syntax allows different settings to be
-provided per NIC.
-
-@subsection -smb (since 2.6.0)
-
-The ``-smb /some/dir'' argument is replaced by either
-``-netdev user,id=x,smb=/some/dir '' (for pluggable NICs, accompanied
-with ``-device ...,netdev=x''), or ``-nic user,smb=/some/dir''
-(for embedded NICs). The new syntax allows different settings to be
-provided per NIC.
-
 @subsection -usbdevice (since 2.10.0)
 
 The ``-usbdevice DEV'' argument is now a synonym for setting
diff --git a/qemu-options.hx b/qemu-options.hx
index 7ca539a..a642ad2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1813,16 +1813,6 @@ STEXI
 @table @option
 ETEXI
 
-HXCOMM Legacy slirp options (now moved to -net user):
-#ifdef CONFIG_SLIRP
-DEF("tftp", HAS_ARG, QEMU_OPTION_tftp, "", QEMU_ARCH_ALL)
-DEF("bootp", HAS_ARG, QEMU_OPTION_bootp, "", QEMU_ARCH_ALL)
-DEF("redir", HAS_ARG, QEMU_OPTION_redir, "", QEMU_ARCH_ALL)
-#ifndef _WIN32
-DEF("smb", HAS_ARG, QEMU_OPTION_smb, "", QEMU_ARCH_ALL)
-#endif
-#endif
-
 DEF("netdev", HAS_ARG, QEMU_OPTION_netdev,
 #ifdef CONFIG_SLIRP
     "-netdev user,id=str[,ipv4[=on|off]][,net=addr[/mask]][,host=addr]\n"
@@ -2150,11 +2140,6 @@ qemu-system-i386 -nic  'user,id=n1,guestfwd=tcp:10.0.2.100:1234-cmd:netcat 10.10
 
 @end table
 
-Note: Legacy stand-alone options -tftp, -bootp, -smb and -redir are still
-processed and applied to -net user. Mixing them with the new configuration
-syntax gives undefined results. Their use for new applications is discouraged
-as they will be removed from future versions.
-
 @item -netdev tap,id=@var{id}[,fd=@var{h}][,ifname=@var{name}][,script=@var{file}][,downscript=@var{dfile}][,br=@var{bridge}][,helper=@var{helper}]
 Configure a host TAP network backend with ID @var{id}.
 
diff --git a/vl.c b/vl.c
index cfeee0d..1b8ff85 100644
--- a/vl.c
+++ b/vl.c
@@ -3168,24 +3168,6 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
 #endif
-#ifdef CONFIG_SLIRP
-            case QEMU_OPTION_tftp:
-                error_report("The -tftp option is deprecated. "
-                             "Please use '-netdev user,tftp=...' instead.");
-                legacy_tftp_prefix = optarg;
-                break;
-            case QEMU_OPTION_bootp:
-                error_report("The -bootp option is deprecated. "
-                             "Please use '-netdev user,bootfile=...' instead.");
-                legacy_bootp_filename = optarg;
-                break;
-            case QEMU_OPTION_redir:
-                error_report("The -redir option is deprecated. "
-                             "Please use '-netdev user,hostfwd=...' instead.");
-                if (net_slirp_redir(optarg) < 0)
-                    exit(1);
-                break;
-#endif
             case QEMU_OPTION_bt:
                 add_device_config(DEV_BT, optarg);
                 break;
-- 
1.8.3.1

[Qemu-devel] [PULL 5/9] tests/libqos: Utilize newer glib spawn check

[Thomas Huth]

From: Eric Blake <eblake@redhat.com>

During development, I got a 'make check' failure that claimed:

qemu-img returned status code 32512
**
ERROR:tests/libqos/libqos.c:202:mkimg: assertion failed: (!rc)

But 32512 is too big for a normal exit status value, which means we
failed to use WEXITSTATUS() to shift the bits to the desired value
for printing.  However, instead of worrying about how to portably
parse g_spawn()'s rc in the proper platform-dependent manner, it's
better to just rely on the fact that we now require glib 2.40 (since
commit e7b3af815) and can therefore use glib's portable checker
instead, where the message under my same condition improves to:

Child process exited with code 127
**
ERROR:tests/libqos/libqos.c:192:mkimg: assertion failed: (ret && !err)

Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/libqos/libqos.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/tests/libqos/libqos.c b/tests/libqos/libqos.c
index 013ca68..c514187 100644
--- a/tests/libqos/libqos.c
+++ b/tests/libqos/libqos.c
@@ -185,22 +185,12 @@ void mkimg(const char *file, const char *fmt, unsigned size_mb)
     cli = g_strdup_printf("%s create -f %s %s %uM", qemu_img_abs_path,
                           fmt, file, size_mb);
     ret = g_spawn_command_line_sync(cli, &out, &out2, &rc, &err);
-    if (err) {
+    if (err || !g_spawn_check_exit_status(rc, &err)) {
         fprintf(stderr, "%s\n", err->message);
         g_error_free(err);
     }
     g_assert(ret && !err);
 
-    /* In glib 2.34, we have g_spawn_check_exit_status. in 2.12, we don't.
-     * glib 2.43.91 implementation assumes that any non-zero is an error for
-     * windows, but uses extra precautions for Linux. However,
-     * 0 is only possible if the program exited normally, so that should be
-     * sufficient for our purposes on all platforms, here. */
-    if (rc) {
-        fprintf(stderr, "qemu-img returned status code %d\n", rc);
-    }
-    g_assert(!rc);
-
     g_free(out);
     g_free(out2);
     g_free(cli);
-- 
1.8.3.1

[Qemu-devel] [PULL 6/9] tests: add qmp_assert_error_class()

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

This helper will simplify a bunch of code checking for QMP errors and
can be shared by various tests.  Note that test-qga does check for
error description as well, so don't replace the code there for now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/drive_del-test.c |  5 +---
 tests/libqtest.c       | 11 ++++++++
 tests/libqtest.h       |  9 +++++++
 tests/qmp-test.c       | 73 ++++++++++++++++----------------------------------
 tests/test-qga.c       |  9 ++-----
 5 files changed, 46 insertions(+), 61 deletions(-)

diff --git a/tests/drive_del-test.c b/tests/drive_del-test.c
index 673c101..4a1a088 100644
--- a/tests/drive_del-test.c
+++ b/tests/drive_del-test.c
@@ -67,7 +67,6 @@ static void test_after_failed_device_add(void)
 {
     char driver[32];
     QDict *response;
-    QDict *error;
 
     snprintf(driver, sizeof(driver), "virtio-blk-%s",
              qvirtio_get_dev_type());
@@ -83,9 +82,7 @@ static void test_after_failed_device_add(void)
                    "   'drive': 'drive0'"
                    "}}", driver);
     g_assert(response);
-    error = qdict_get_qdict(response, "error");
-    g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, "GenericError");
-    qobject_unref(response);
+    qmp_assert_error_class(response, "GenericError");
 
     /* Delete the drive */
     drive_del();
diff --git a/tests/libqtest.c b/tests/libqtest.c
index d635c5b..2cd5736 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -1194,3 +1194,14 @@ bool qmp_rsp_is_err(QDict *rsp)
     qobject_unref(rsp);
     return !!error;
 }
+
+void qmp_assert_error_class(QDict *rsp, const char *class)
+{
+    QDict *error = qdict_get_qdict(rsp, "error");
+
+    g_assert_cmpstr(qdict_get_try_str(error, "class"), ==, class);
+    g_assert_nonnull(qdict_get_try_str(error, "desc"));
+    g_assert(!qdict_haskey(rsp, "return"));
+
+    qobject_unref(rsp);
+}
diff --git a/tests/libqtest.h b/tests/libqtest.h
index 36d5cae..ed88ff9 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -1004,4 +1004,13 @@ void qtest_qmp_device_del(const char *id);
  */
 bool qmp_rsp_is_err(QDict *rsp);
 
+/**
+ * qmp_assert_error_class:
+ * @rsp: QMP response to check for error
+ * @class: an error class
+ *
+ * Assert the response has the given error class and discard @rsp.
+ */
+void qmp_assert_error_class(QDict *rsp, const char *class);
+
 #endif
diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index 4ae2245..b347228 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -21,15 +21,6 @@
 
 const char common_args[] = "-nodefaults -machine none";
 
-static const char *get_error_class(QDict *resp)
-{
-    QDict *error = qdict_get_qdict(resp, "error");
-    const char *desc = qdict_get_try_str(error, "desc");
-
-    g_assert(desc);
-    return error ? qdict_get_try_str(error, "class") : NULL;
-}
-
 static void test_version(QObject *version)
 {
     Visitor *v;
@@ -42,15 +33,12 @@ static void test_version(QObject *version)
     visit_free(v);
 }
 
-static bool recovered(QTestState *qts)
+static void assert_recovered(QTestState *qts)
 {
     QDict *resp;
-    bool ret;
 
     resp = qtest_qmp(qts, "{ 'execute': 'no-such-cmd' }");
-    ret = !strcmp(get_error_class(resp), "CommandNotFound");
-    qobject_unref(resp);
-    return ret;
+    qmp_assert_error_class(resp, "CommandNotFound");
 }
 
 static void test_malformed(QTestState *qts)
@@ -60,73 +48,61 @@ static void test_malformed(QTestState *qts)
     /* syntax error */
     qtest_qmp_send_raw(qts, "{]\n");
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
-    g_assert(recovered(qts));
+    qmp_assert_error_class(resp, "GenericError");
+    assert_recovered(qts);
 
     /* lexical error: impossible byte outside string */
     qtest_qmp_send_raw(qts, "{\xFF");
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
-    g_assert(recovered(qts));
+    qmp_assert_error_class(resp, "GenericError");
+    assert_recovered(qts);
 
     /* lexical error: funny control character outside string */
     qtest_qmp_send_raw(qts, "{\x01");
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
-    g_assert(recovered(qts));
+    qmp_assert_error_class(resp, "GenericError");
+    assert_recovered(qts);
 
     /* lexical error: impossible byte in string */
     qtest_qmp_send_raw(qts, "{'bad \xFF");
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
-    g_assert(recovered(qts));
+    qmp_assert_error_class(resp, "GenericError");
+    assert_recovered(qts);
 
     /* lexical error: control character in string */
     qtest_qmp_send_raw(qts, "{'execute': 'nonexistent', 'id':'\n");
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
-    g_assert(recovered(qts));
+    qmp_assert_error_class(resp, "GenericError");
+    assert_recovered(qts);
 
     /* lexical error: interpolation */
     qtest_qmp_send_raw(qts, "%%p\n");
     /* two errors, one for "%", one for "p" */
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
     resp = qtest_qmp_receive(qts);
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
-    g_assert(recovered(qts));
+    qmp_assert_error_class(resp, "GenericError");
+    assert_recovered(qts);
 
     /* Not even a dictionary */
     resp = qtest_qmp(qts, "null");
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
 
     /* No "execute" key */
     resp = qtest_qmp(qts, "{}");
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
 
     /* "execute" isn't a string */
     resp = qtest_qmp(qts, "{ 'execute': true }");
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
 
     /* "arguments" isn't a dictionary */
     resp = qtest_qmp(qts, "{ 'execute': 'no-such-cmd', 'arguments': [] }");
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
 
     /* extra key */
     resp = qtest_qmp(qts, "{ 'execute': 'no-such-cmd', 'extra': true }");
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
 }
 
 static void test_qmp_protocol(void)
@@ -148,8 +124,7 @@ static void test_qmp_protocol(void)
 
     /* Test valid command before handshake */
     resp = qtest_qmp(qts, "{ 'execute': 'query-version' }");
-    g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "CommandNotFound");
 
     /* Test malformed commands before handshake */
     test_malformed(qts);
@@ -162,8 +137,7 @@ static void test_qmp_protocol(void)
 
     /* Test repeated handshake */
     resp = qtest_qmp(qts, "{ 'execute': 'qmp_capabilities' }");
-    g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "CommandNotFound");
 
     /* Test valid command */
     resp = qtest_qmp(qts, "{ 'execute': 'query-version' }");
@@ -182,9 +156,8 @@ static void test_qmp_protocol(void)
 
     /* Test command failure with 'id' */
     resp = qtest_qmp(qts, "{ 'execute': 'human-monitor-command', 'id': 2 }");
-    g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
     g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
-    qobject_unref(resp);
+    qmp_assert_error_class(resp, "GenericError");
 
     qtest_quit(qts);
 }
diff --git a/tests/test-qga.c b/tests/test-qga.c
index f69cdf6..3d63774 100644
--- a/tests/test-qga.c
+++ b/tests/test-qga.c
@@ -244,17 +244,12 @@ static void test_qga_invalid_id(gconstpointer fix)
 static void test_qga_invalid_oob(gconstpointer fix)
 {
     const TestFixture *fixture = fix;
-    QDict *ret, *error;
-    const char *class;
+    QDict *ret;
 
     ret = qmp_fd(fixture->fd, "{'exec-oob': 'guest-ping'}");
     g_assert_nonnull(ret);
 
-    error = qdict_get_qdict(ret, "error");
-    class = qdict_get_try_str(error, "class");
-    g_assert_cmpstr(class, ==, "GenericError");
-
-    qobject_unref(ret);
+    qmp_assert_error_class(ret, "GenericError");
 }
 
 static void test_qga_invalid_args(gconstpointer fix)
-- 
1.8.3.1

[Qemu-devel] [PULL 7/9] tests: add qmp/object-add-without-props test

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

test_object_add_without_props() tests a bug in qmp_object_add() we
fixed in commit e64c75a975.  Sadly, we don't have systematic
object-add tests.  This lone test can go into qmp-cmd-test for want of
a better home.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qmp-cmd-test.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tests/qmp-cmd-test.c b/tests/qmp-cmd-test.c
index c5b70df..d12cac5 100644
--- a/tests/qmp-cmd-test.c
+++ b/tests/qmp-cmd-test.c
@@ -197,6 +197,19 @@ static void add_query_tests(QmpSchema *schema)
     }
 }
 
+static void test_object_add_without_props(void)
+{
+    QTestState *qts;
+    QDict *resp;
+
+    qts = qtest_init(common_args);
+    resp = qtest_qmp(qts, "{'execute': 'object-add', 'arguments':"
+                    " {'qom-type': 'memory-backend-ram', 'id': 'ram1' } }");
+    g_assert_nonnull(resp);
+    qmp_assert_error_class(resp, "GenericError");
+    qtest_quit(qts);
+}
+
 int main(int argc, char *argv[])
 {
     QmpSchema schema;
@@ -206,6 +219,11 @@ int main(int argc, char *argv[])
 
     qmp_schema_init(&schema);
     add_query_tests(&schema);
+
+    qtest_add_func("qmp/object-add-without-props",
+                   test_object_add_without_props);
+    /* TODO: add coverage of generic object-add failure modes */
+
     ret = g_test_run();
 
     qmp_schema_cleanup(&schema);
-- 
1.8.3.1

[Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

test_qom_set_without_value() is about a bug in infrastructure used by
the QMP core, fixed in commit c489780203.  We covered the bug in
infrastructure unit tests (commit bce3035a44).  I wrote that test
earlier, to cover QMP level as well, the test could go into qmp-test.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qmp-test.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tests/qmp-test.c b/tests/qmp-test.c
index b347228..2b923f0 100644
--- a/tests/qmp-test.c
+++ b/tests/qmp-test.c
@@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
     qtest_quit(qs);
 }
 
+static void test_qom_set_without_value(void)
+{
+    QTestState *qts;
+    QDict *resp;
+
+    qts = qtest_init(common_args);
+    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
+                     " { 'path': '/machine', 'property': 'rtc-time' } }");
+    g_assert_nonnull(resp);
+    qmp_assert_error_class(resp, "GenericError");
+    qtest_quit(qts);
+}
+
 int main(int argc, char *argv[])
 {
     g_test_init(&argc, &argv, NULL);
@@ -328,6 +341,7 @@ int main(int argc, char *argv[])
     qtest_add_func("qmp/protocol", test_qmp_protocol);
     qtest_add_func("qmp/oob", test_qmp_oob);
     qtest_add_func("qmp/preconfig", test_qmp_preconfig);
+    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
 
     return g_test_run();
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 9/9] tests: add a qmp success-response test

[Thomas Huth]

From: Marc-André Lureau <marcandre.lureau@redhat.com>

Verify the usage of this schema feature and the API behaviour.  This
should be the only case where qmp_dispatch() returns NULL.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 tests/qapi-schema/qapi-schema-test.json |  2 ++
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 tests/test-qmp-cmds.c                   | 17 +++++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 11aa4c8..fb03163 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -137,6 +137,8 @@
   'data': {'ud1a': 'UserDefOne', '*ud1b': 'UserDefOne'},
   'returns': 'UserDefTwo' }
 
+{ 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+
 # Returning a non-dictionary requires a name from the whitelist
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
   'returns': 'int' }
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 0da9245..218ac7d 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -156,6 +156,8 @@ object q_obj_user_def_cmd2-arg
     member ud1b: UserDefOne optional=True
 command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
    gen=True success_response=True boxed=False oob=False preconfig=False
+command cmd-success-response None -> None
+   gen=True success_response=False boxed=False oob=False preconfig=False
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index ab414fa..4ab2b6e 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -32,6 +32,10 @@ void qmp_test_flags_command(Error **errp)
 {
 }
 
+void qmp_cmd_success_response(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
@@ -153,6 +157,17 @@ static void test_dispatch_cmd_failure(void)
     qobject_unref(req);
 }
 
+static void test_dispatch_cmd_success_response(void)
+{
+    QDict *req = qdict_new();
+    QDict *resp;
+
+    qdict_put_str(req, "execute", "cmd-success-response");
+    resp = qmp_dispatch(&qmp_commands, QOBJECT(req), false);
+    g_assert_null(resp);
+    qobject_unref(req);
+}
+
 static QObject *test_qmp_dispatch(QDict *req)
 {
     QDict *resp;
@@ -289,6 +304,8 @@ int main(int argc, char **argv)
     g_test_add_func("/qmp/dispatch_cmd", test_dispatch_cmd);
     g_test_add_func("/qmp/dispatch_cmd_failure", test_dispatch_cmd_failure);
     g_test_add_func("/qmp/dispatch_cmd_io", test_dispatch_cmd_io);
+    g_test_add_func("/qmp/dispatch_cmd_success_response",
+                    test_dispatch_cmd_success_response);
     g_test_add_func("/qmp/dealloc_types", test_dealloc_types);
     g_test_add_func("/qmp/dealloc_partial", test_dealloc_partial);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> test_qom_set_without_value() is about a bug in infrastructure used by
> the QMP core, fixed in commit c489780203.  We covered the bug in
> infrastructure unit tests (commit bce3035a44).  I wrote that test
> earlier, to cover QMP level as well, the test could go into qmp-test.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  tests/qmp-test.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> index b347228..2b923f0 100644
> --- a/tests/qmp-test.c
> +++ b/tests/qmp-test.c
> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>      qtest_quit(qs);
>  }
>  
> +static void test_qom_set_without_value(void)
> +{
> +    QTestState *qts;
> +    QDict *resp;
> +
> +    qts = qtest_init(common_args);
> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
> +    g_assert_nonnull(resp);
> +    qmp_assert_error_class(resp, "GenericError");
> +    qtest_quit(qts);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      g_test_init(&argc, &argv, NULL);
> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>      qtest_add_func("qmp/oob", test_qmp_oob);
>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>  
>      return g_test_run();
>  }

I'm afraid you missed my objection to naming:
Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html

If you could work that into PULL v2, I'd be obliged.  If not, I'll have
to address it in a follow-up patch.

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Thomas Huth]

On 2018-08-31 14:04, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> test_qom_set_without_value() is about a bug in infrastructure used by
>> the QMP core, fixed in commit c489780203.  We covered the bug in
>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  tests/qmp-test.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> index b347228..2b923f0 100644
>> --- a/tests/qmp-test.c
>> +++ b/tests/qmp-test.c
>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>>      qtest_quit(qs);
>>  }
>>  
>> +static void test_qom_set_without_value(void)
>> +{
>> +    QTestState *qts;
>> +    QDict *resp;
>> +
>> +    qts = qtest_init(common_args);
>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
>> +    g_assert_nonnull(resp);
>> +    qmp_assert_error_class(resp, "GenericError");
>> +    qtest_quit(qts);
>> +}
>> +
>>  int main(int argc, char *argv[])
>>  {
>>      g_test_init(&argc, &argv, NULL);
>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>      qtest_add_func("qmp/oob", test_qmp_oob);
>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>  
>>      return g_test_run();
>>  }
> 
> I'm afraid you missed my objection to naming:
> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html

Sorry about that, I was not on CC: for that series. I used the patches
from v5 where Marc-André put me on CC:.

> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
> to address it in a follow-up patch.

IMHO the naming is not that bad ... OTOH, I think Peter might already be
away? ... so we've got plenty of time to sort this out anyway.
Marc-André, could you send a new version of the patch?

 Thomas

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Marc-André Lureau]

Hi
On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 2018-08-31 14:04, Markus Armbruster wrote:
> > Thomas Huth <thuth@redhat.com> writes:
> >
> >> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >> test_qom_set_without_value() is about a bug in infrastructure used by
> >> the QMP core, fixed in commit c489780203.  We covered the bug in
> >> infrastructure unit tests (commit bce3035a44).  I wrote that test
> >> earlier, to cover QMP level as well, the test could go into qmp-test.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >> ---
> >>  tests/qmp-test.c | 14 ++++++++++++++
> >>  1 file changed, 14 insertions(+)
> >>
> >> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> >> index b347228..2b923f0 100644
> >> --- a/tests/qmp-test.c
> >> +++ b/tests/qmp-test.c
> >> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
> >>      qtest_quit(qs);
> >>  }
> >>
> >> +static void test_qom_set_without_value(void)
> >> +{
> >> +    QTestState *qts;
> >> +    QDict *resp;
> >> +
> >> +    qts = qtest_init(common_args);
> >> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> >> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
> >> +    g_assert_nonnull(resp);
> >> +    qmp_assert_error_class(resp, "GenericError");
> >> +    qtest_quit(qts);
> >> +}
> >> +
> >>  int main(int argc, char *argv[])
> >>  {
> >>      g_test_init(&argc, &argv, NULL);
> >> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
> >>      qtest_add_func("qmp/protocol", test_qmp_protocol);
> >>      qtest_add_func("qmp/oob", test_qmp_oob);
> >>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> >> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
> >>
> >>      return g_test_run();
> >>  }
> >
> > I'm afraid you missed my objection to naming:
> > Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
> > https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
>
> Sorry about that, I was not on CC: for that series. I used the patches
> from v5 where Marc-André put me on CC:.
>
> > If you could work that into PULL v2, I'd be obliged.  If not, I'll have
> > to address it in a follow-up patch.
>
> IMHO the naming is not that bad ... OTOH, I think Peter might already be
> away? ... so we've got plenty of time to sort this out anyway.
> Marc-André, could you send a new version of the patch?

Tbh, I don't care so much about the naming of the test, but (for once)
I respectfully don't think Markus suggestion is better.

The function checks "qom-set" without 'value' argument:
"qom-set-without-value", no brainer..

Naming it "invalid-arg" is so generic that I wouldn't be able what it does.


-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Thomas Huth]

On 2018-08-31 15:24, Marc-André Lureau wrote:
> Hi
> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 2018-08-31 14:04, Markus Armbruster wrote:
>>> Thomas Huth <thuth@redhat.com> writes:
>>>
>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>
>>>> test_qom_set_without_value() is about a bug in infrastructure used by
>>>> the QMP core, fixed in commit c489780203.  We covered the bug in
>>>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>>>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  tests/qmp-test.c | 14 ++++++++++++++
>>>>  1 file changed, 14 insertions(+)
>>>>
>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>>>> index b347228..2b923f0 100644
>>>> --- a/tests/qmp-test.c
>>>> +++ b/tests/qmp-test.c
>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>>>>      qtest_quit(qs);
>>>>  }
>>>>
>>>> +static void test_qom_set_without_value(void)
>>>> +{
>>>> +    QTestState *qts;
>>>> +    QDict *resp;
>>>> +
>>>> +    qts = qtest_init(common_args);
>>>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>>>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
>>>> +    g_assert_nonnull(resp);
>>>> +    qmp_assert_error_class(resp, "GenericError");
>>>> +    qtest_quit(qts);
>>>> +}
>>>> +
>>>>  int main(int argc, char *argv[])
>>>>  {
>>>>      g_test_init(&argc, &argv, NULL);
>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
>>>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>>>      qtest_add_func("qmp/oob", test_qmp_oob);
>>>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>>>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>>>
>>>>      return g_test_run();
>>>>  }
>>>
>>> I'm afraid you missed my objection to naming:
>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
>>
>> Sorry about that, I was not on CC: for that series. I used the patches
>> from v5 where Marc-André put me on CC:.
>>
>>> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
>>> to address it in a follow-up patch.
>>
>> IMHO the naming is not that bad ... OTOH, I think Peter might already be
>> away? ... so we've got plenty of time to sort this out anyway.
>> Marc-André, could you send a new version of the patch?
> 
> Tbh, I don't care so much about the naming of the test, but (for once)
> I respectfully don't think Markus suggestion is better.
> 
> The function checks "qom-set" without 'value' argument:
> "qom-set-without-value", no brainer..
> 
> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.

Ok, then let's keep it this way. As I said, IMHO the current naming is
not really bad, and I also don't have any suggestions for a perfect name
right now.

 Thomas

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 2018-08-31 15:24, Marc-André Lureau wrote:
>> Hi
>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
>>>
>>> On 2018-08-31 14:04, Markus Armbruster wrote:
>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> test_qom_set_without_value() is about a bug in infrastructure used by
>>>>> the QMP core, fixed in commit c489780203.  We covered the bug in
>>>>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>>>>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  tests/qmp-test.c | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>>>>> index b347228..2b923f0 100644
>>>>> --- a/tests/qmp-test.c
>>>>> +++ b/tests/qmp-test.c
>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>>>>>      qtest_quit(qs);
>>>>>  }
>>>>>
>>>>> +static void test_qom_set_without_value(void)
>>>>> +{
>>>>> +    QTestState *qts;
>>>>> +    QDict *resp;
>>>>> +
>>>>> +    qts = qtest_init(common_args);
>>>>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>>>>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
>>>>> +    g_assert_nonnull(resp);
>>>>> +    qmp_assert_error_class(resp, "GenericError");
>>>>> +    qtest_quit(qts);
>>>>> +}
>>>>> +
>>>>>  int main(int argc, char *argv[])
>>>>>  {
>>>>>      g_test_init(&argc, &argv, NULL);
>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
>>>>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>>>>      qtest_add_func("qmp/oob", test_qmp_oob);
>>>>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>>>>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>>>>
>>>>>      return g_test_run();
>>>>>  }
>>>>
>>>> I'm afraid you missed my objection to naming:
>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
>>>
>>> Sorry about that, I was not on CC: for that series. I used the patches
>>> from v5 where Marc-André put me on CC:.
>>>
>>>> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
>>>> to address it in a follow-up patch.
>>>
>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be
>>> away? ... so we've got plenty of time to sort this out anyway.
>>> Marc-André, could you send a new version of the patch?
>> 
>> Tbh, I don't care so much about the naming of the test, but (for once)
>> I respectfully don't think Markus suggestion is better.
>> 
>> The function checks "qom-set" without 'value' argument:
>> "qom-set-without-value", no brainer..

Nope, that's not what it tests.  It tests the visitor, the marshalling
code generator, and the QMP core handle a certain kind of invalid
arguments correctly.  It does not test qom-set.  I explained all that
already.

>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.

I can accept "missing-any" or "missing-any-arg".  I object to any name
involving qom-set, because the test is not about qom-set at all.

If it was, then putting it in qmp-test.c would be wrong.

> Ok, then let's keep it this way. As I said, IMHO the current naming is
> not really bad, and I also don't have any suggestions for a perfect name
> right now.

We don't need a perfect name.  We need one that's not actively
misleading.

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Thomas Huth]

On 2018-08-31 16:35, Markus Armbruster wrote:
> Thomas Huth <thuth@redhat.com> writes:
> 
>> On 2018-08-31 15:24, Marc-André Lureau wrote:
>>> Hi
>>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 2018-08-31 14:04, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>
>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>
>>>>>> test_qom_set_without_value() is about a bug in infrastructure used by
>>>>>> the QMP core, fixed in commit c489780203.  We covered the bug in
>>>>>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>>>>>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>>>>>
>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>  tests/qmp-test.c | 14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>>>>>> index b347228..2b923f0 100644
>>>>>> --- a/tests/qmp-test.c
>>>>>> +++ b/tests/qmp-test.c
>>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>>>>>>      qtest_quit(qs);
>>>>>>  }
>>>>>>
>>>>>> +static void test_qom_set_without_value(void)
>>>>>> +{
>>>>>> +    QTestState *qts;
>>>>>> +    QDict *resp;
>>>>>> +
>>>>>> +    qts = qtest_init(common_args);
>>>>>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>>>>>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
>>>>>> +    g_assert_nonnull(resp);
>>>>>> +    qmp_assert_error_class(resp, "GenericError");
>>>>>> +    qtest_quit(qts);
>>>>>> +}
>>>>>> +
>>>>>>  int main(int argc, char *argv[])
>>>>>>  {
>>>>>>      g_test_init(&argc, &argv, NULL);
>>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
>>>>>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>>>>>      qtest_add_func("qmp/oob", test_qmp_oob);
>>>>>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>>>>>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>>>>>
>>>>>>      return g_test_run();
>>>>>>  }
>>>>>
>>>>> I'm afraid you missed my objection to naming:
>>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
>>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
>>>>
>>>> Sorry about that, I was not on CC: for that series. I used the patches
>>>> from v5 where Marc-André put me on CC:.
>>>>
>>>>> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
>>>>> to address it in a follow-up patch.
>>>>
>>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be
>>>> away? ... so we've got plenty of time to sort this out anyway.
>>>> Marc-André, could you send a new version of the patch?
>>>
>>> Tbh, I don't care so much about the naming of the test, but (for once)
>>> I respectfully don't think Markus suggestion is better.
>>>
>>> The function checks "qom-set" without 'value' argument:
>>> "qom-set-without-value", no brainer..
> 
> Nope, that's not what it tests.  It tests the visitor, the marshalling
> code generator, and the QMP core handle a certain kind of invalid
> arguments correctly.  It does not test qom-set.  I explained all that
> already.
> 
>>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.
> 
> I can accept "missing-any" or "missing-any-arg".  I object to any name
> involving qom-set, because the test is not about qom-set at all.
> 
> If it was, then putting it in qmp-test.c would be wrong.
> 
>> Ok, then let's keep it this way. As I said, IMHO the current naming is
>> not really bad, and I also don't have any suggestions for a perfect name
>> right now.
> 
> We don't need a perfect name.  We need one that's not actively
> misleading.

Ok, then let's cancel this PULL request. I'll send a new one after the
"vacation freeze" (i.e. in three weeks), that should be enough time for
both of you to come to an agreement about the best naming here.

 Thomas

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Thomas Huth <thuth@redhat.com> writes:
>
>> On 2018-08-31 15:24, Marc-André Lureau wrote:
>>> Hi
>>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 2018-08-31 14:04, Markus Armbruster wrote:
>>>>> Thomas Huth <thuth@redhat.com> writes:
>>>>>
>>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>>
>>>>>> test_qom_set_without_value() is about a bug in infrastructure used by
>>>>>> the QMP core, fixed in commit c489780203.  We covered the bug in
>>>>>> infrastructure unit tests (commit bce3035a44).  I wrote that test
>>>>>> earlier, to cover QMP level as well, the test could go into qmp-test.
>>>>>>
>>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
>>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>>> ---
>>>>>>  tests/qmp-test.c | 14 ++++++++++++++
>>>>>>  1 file changed, 14 insertions(+)
>>>>>>
>>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>>>>>> index b347228..2b923f0 100644
>>>>>> --- a/tests/qmp-test.c
>>>>>> +++ b/tests/qmp-test.c
>>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
>>>>>>      qtest_quit(qs);
>>>>>>  }
>>>>>>
>>>>>> +static void test_qom_set_without_value(void)
>>>>>> +{
>>>>>> +    QTestState *qts;
>>>>>> +    QDict *resp;
>>>>>> +
>>>>>> +    qts = qtest_init(common_args);
>>>>>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
>>>>>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
>>>>>> +    g_assert_nonnull(resp);
>>>>>> +    qmp_assert_error_class(resp, "GenericError");
>>>>>> +    qtest_quit(qts);
>>>>>> +}
>>>>>> +
>>>>>>  int main(int argc, char *argv[])
>>>>>>  {
>>>>>>      g_test_init(&argc, &argv, NULL);
>>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
>>>>>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
>>>>>>      qtest_add_func("qmp/oob", test_qmp_oob);
>>>>>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
>>>>>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
>>>>>>
>>>>>>      return g_test_run();
>>>>>>  }
>>>>>
>>>>> I'm afraid you missed my objection to naming:
>>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
>>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
>>>>
>>>> Sorry about that, I was not on CC: for that series. I used the patches
>>>> from v5 where Marc-André put me on CC:.
>>>>
>>>>> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
>>>>> to address it in a follow-up patch.
>>>>
>>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be
>>>> away? ... so we've got plenty of time to sort this out anyway.
>>>> Marc-André, could you send a new version of the patch?
>>> 
>>> Tbh, I don't care so much about the naming of the test, but (for once)
>>> I respectfully don't think Markus suggestion is better.
>>> 
>>> The function checks "qom-set" without 'value' argument:
>>> "qom-set-without-value", no brainer..
>
> Nope, that's not what it tests.  It tests the visitor, the marshalling
> code generator, and the QMP core handle a certain kind of invalid
> arguments correctly.  It does not test qom-set.  I explained all that
> already.
>
>>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.
>
> I can accept "missing-any" or "missing-any-arg".  I object to any name
> involving qom-set, because the test is not about qom-set at all.
>
> If it was, then putting it in qmp-test.c would be wrong.
>
>> Ok, then let's keep it this way. As I said, IMHO the current naming is
>> not really bad, and I also don't have any suggestions for a perfect name
>> right now.
>
> We don't need a perfect name.  We need one that's not actively
> misleading.

Marc-André, would "qmp/missing-any-arg" and test_missing_any_arg() work
for you?

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Marc-André Lureau]

Hi

On Tue, Oct 9, 2018 at 8:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Thomas Huth <thuth@redhat.com> writes:
> >
> >> On 2018-08-31 15:24, Marc-André Lureau wrote:
> >>> Hi
> >>> On Fri, Aug 31, 2018 at 3:18 PM Thomas Huth <thuth@redhat.com> wrote:
> >>>>
> >>>> On 2018-08-31 14:04, Markus Armbruster wrote:
> >>>>> Thomas Huth <thuth@redhat.com> writes:
> >>>>>
> >>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>>
> >>>>>> test_qom_set_without_value() is about a bug in infrastructure used by
> >>>>>> the QMP core, fixed in commit c489780203.  We covered the bug in
> >>>>>> infrastructure unit tests (commit bce3035a44).  I wrote that test
> >>>>>> earlier, to cover QMP level as well, the test could go into qmp-test.
> >>>>>>
> >>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>>>>> Reviewed-by: Thomas Huth <thuth@redhat.com>
> >>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> >>>>>> ---
> >>>>>>  tests/qmp-test.c | 14 ++++++++++++++
> >>>>>>  1 file changed, 14 insertions(+)
> >>>>>>
> >>>>>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> >>>>>> index b347228..2b923f0 100644
> >>>>>> --- a/tests/qmp-test.c
> >>>>>> +++ b/tests/qmp-test.c
> >>>>>> @@ -321,6 +321,19 @@ static void test_qmp_preconfig(void)
> >>>>>>      qtest_quit(qs);
> >>>>>>  }
> >>>>>>
> >>>>>> +static void test_qom_set_without_value(void)
> >>>>>> +{
> >>>>>> +    QTestState *qts;
> >>>>>> +    QDict *resp;
> >>>>>> +
> >>>>>> +    qts = qtest_init(common_args);
> >>>>>> +    resp = qtest_qmp(qts, "{'execute': 'qom-set', 'arguments':"
> >>>>>> +                     " { 'path': '/machine', 'property': 'rtc-time' } }");
> >>>>>> +    g_assert_nonnull(resp);
> >>>>>> +    qmp_assert_error_class(resp, "GenericError");
> >>>>>> +    qtest_quit(qts);
> >>>>>> +}
> >>>>>> +
> >>>>>>  int main(int argc, char *argv[])
> >>>>>>  {
> >>>>>>      g_test_init(&argc, &argv, NULL);
> >>>>>> @@ -328,6 +341,7 @@ int main(int argc, char *argv[])
> >>>>>>      qtest_add_func("qmp/protocol", test_qmp_protocol);
> >>>>>>      qtest_add_func("qmp/oob", test_qmp_oob);
> >>>>>>      qtest_add_func("qmp/preconfig", test_qmp_preconfig);
> >>>>>> +    qtest_add_func("qmp/qom-set-without-value", test_qom_set_without_value);
> >>>>>>
> >>>>>>      return g_test_run();
> >>>>>>  }
> >>>>>
> >>>>> I'm afraid you missed my objection to naming:
> >>>>> Message-ID: <8736uvujxx.fsf@dusky.pond.sub.org>
> >>>>> https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg06368.html
> >>>>
> >>>> Sorry about that, I was not on CC: for that series. I used the patches
> >>>> from v5 where Marc-André put me on CC:.
> >>>>
> >>>>> If you could work that into PULL v2, I'd be obliged.  If not, I'll have
> >>>>> to address it in a follow-up patch.
> >>>>
> >>>> IMHO the naming is not that bad ... OTOH, I think Peter might already be
> >>>> away? ... so we've got plenty of time to sort this out anyway.
> >>>> Marc-André, could you send a new version of the patch?
> >>>
> >>> Tbh, I don't care so much about the naming of the test, but (for once)
> >>> I respectfully don't think Markus suggestion is better.
> >>>
> >>> The function checks "qom-set" without 'value' argument:
> >>> "qom-set-without-value", no brainer..
> >
> > Nope, that's not what it tests.  It tests the visitor, the marshalling
> > code generator, and the QMP core handle a certain kind of invalid
> > arguments correctly.  It does not test qom-set.  I explained all that
> > already.
> >
> >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.
> >
> > I can accept "missing-any" or "missing-any-arg".  I object to any name
> > involving qom-set, because the test is not about qom-set at all.
> >
> > If it was, then putting it in qmp-test.c would be wrong.
> >
> >> Ok, then let's keep it this way. As I said, IMHO the current naming is
> >> not really bad, and I also don't have any suggestions for a perfect name
> >> right now.
> >
> > We don't need a perfect name.  We need one that's not actively
> > misleading.
>
> Marc-André, would "qmp/missing-any-arg" and test_missing_any_arg() work
> for you?

Yes, do you want me to update the patch?

-- 
Marc-André Lureau

Re: [Qemu-devel] [PULL 8/9] tests: add qmp/qom-set-without-value test

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Tue, Oct 9, 2018 at 8:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Markus Armbruster <armbru@redhat.com> writes:
>>
>> > Thomas Huth <thuth@redhat.com> writes:
>> >
>> >> On 2018-08-31 15:24, Marc-André Lureau wrote:
[...]
>> >>> Tbh, I don't care so much about the naming of the test, but (for once)
>> >>> I respectfully don't think Markus suggestion is better.
>> >>>
>> >>> The function checks "qom-set" without 'value' argument:
>> >>> "qom-set-without-value", no brainer..
>> >
>> > Nope, that's not what it tests.  It tests the visitor, the marshalling
>> > code generator, and the QMP core handle a certain kind of invalid
>> > arguments correctly.  It does not test qom-set.  I explained all that
>> > already.
>> >
>> >>> Naming it "invalid-arg" is so generic that I wouldn't be able what it does.
>> >
>> > I can accept "missing-any" or "missing-any-arg".  I object to any name
>> > involving qom-set, because the test is not about qom-set at all.
>> >
>> > If it was, then putting it in qmp-test.c would be wrong.
>> >
>> >> Ok, then let's keep it this way. As I said, IMHO the current naming is
>> >> not really bad, and I also don't have any suggestions for a perfect name
>> >> right now.
>> >
>> > We don't need a perfect name.  We need one that's not actively
>> > misleading.
>>
>> Marc-André, would "qmp/missing-any-arg" and test_missing_any_arg() work
>> for you?
>
> Yes, do you want me to update the patch?

Yes, please.