| Message ID | 20180531051641.8473-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: enable OOB by default | expand |
On 05/31/2018 12:16 AM, Peter Xu wrote: > This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a. > > Meanwhile, revert one line from fa198ad9bdef to make sure > qtest_init_without_qmp_handshake() will only pass in one parameter. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > +++ b/tests/libqtest.h > @@ -56,14 +56,11 @@ QTestState *qtest_init(const char *extra_args); > > /** > * qtest_init_without_qmp_handshake: > - * @use_oob: true to have the server advertise OOB support > - * @extra_args: other arguments to pass to QEMU. CAUTION: these > - * arguments are subject to word splitting and shell evaluation. > + * @extra_args: other arguments to pass to QEMU. The CAUTION message was a useful improvement, I don't think we should revert that part of the patch. > @@ -200,13 +199,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, > command = g_strdup_printf("exec %s " > "-qtest unix:%s,nowait " > "-qtest-log %s " > - "-chardev socket,path=%s,nowait,id=char0 " > - "-mon chardev=char0,mode=control%s " > + "-qmp unix:%s,nowait " > "-machine accel=qtest " > "-display none " > "%s", qemu_binary, socket_path, > getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", > - qmp_socket_path, use_oob ? ",x-oob=on" : "", > + qmp_socket_path, I agree with your cover letter that you don't want to check in patch 3/4 as-is, as it temporarily breaks this test. But I disagree with your idea of squashing 3 and 4 into one patch, as having two separate reversion patches that both call out the primary commit that they are reverting is nicer than one patch that reverts two things at once. My suggestion: in patch 3, tweak this line to be: qmp_socket_path, "", so that the test will pass, then in patch 4, remove the useless "" parameter (a minor conflict resolution rather than a clean revert, but we already have that because of fa198ad9b, and I think it results in a nicer git commit history).
On Thu, May 31, 2018 at 09:51:00AM -0500, Eric Blake wrote: > On 05/31/2018 12:16 AM, Peter Xu wrote: > > This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a. > > > > Meanwhile, revert one line from fa198ad9bdef to make sure > > qtest_init_without_qmp_handshake() will only pass in one parameter. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > > +++ b/tests/libqtest.h > > @@ -56,14 +56,11 @@ QTestState *qtest_init(const char *extra_args); > > /** > > * qtest_init_without_qmp_handshake: > > - * @use_oob: true to have the server advertise OOB support > > - * @extra_args: other arguments to pass to QEMU. CAUTION: these > > - * arguments are subject to word splitting and shell evaluation. > > + * @extra_args: other arguments to pass to QEMU. > > The CAUTION message was a useful improvement, I don't think we should revert > that part of the patch. > > > @@ -200,13 +199,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, > > command = g_strdup_printf("exec %s " > > "-qtest unix:%s,nowait " > > "-qtest-log %s " > > - "-chardev socket,path=%s,nowait,id=char0 " > > - "-mon chardev=char0,mode=control%s " > > + "-qmp unix:%s,nowait " > > "-machine accel=qtest " > > "-display none " > > "%s", qemu_binary, socket_path, > > getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", > > - qmp_socket_path, use_oob ? ",x-oob=on" : "", > > + qmp_socket_path, > > I agree with your cover letter that you don't want to check in patch 3/4 > as-is, as it temporarily breaks this test. But I disagree with your idea of > squashing 3 and 4 into one patch, as having two separate reversion patches > that both call out the primary commit that they are reverting is nicer than > one patch that reverts two things at once. My suggestion: in patch 3, tweak > this line to be: > > qmp_socket_path, "", > > so that the test will pass, then in patch 4, remove the useless "" parameter > (a minor conflict resolution rather than a clean revert, but we already have > that because of fa198ad9b, and I think it results in a nicer git commit > history). Will take your suggestion. I'll wait a bit more to see what's the conclusion for "[PATCH] monitor: postpone monitor_qmp_cleanup_queues" before I repost. Thanks for the review.
diff --git a/tests/libqtest.h b/tests/libqtest.h index cbe8df4473..811169453a 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -56,14 +56,11 @@ QTestState *qtest_init(const char *extra_args); /** * qtest_init_without_qmp_handshake: - * @use_oob: true to have the server advertise OOB support - * @extra_args: other arguments to pass to QEMU. CAUTION: these - * arguments are subject to word splitting and shell evaluation. + * @extra_args: other arguments to pass to QEMU. * * Returns: #QTestState instance. */ -QTestState *qtest_init_without_qmp_handshake(bool use_oob, - const char *extra_args); +QTestState *qtest_init_without_qmp_handshake(const char *extra_args); /** * qtest_quit: diff --git a/tests/libqtest.c b/tests/libqtest.c index 43fb97e035..027be0887d 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -166,8 +166,7 @@ static const char *qtest_qemu_binary(void) return qemu_bin; } -QTestState *qtest_init_without_qmp_handshake(bool use_oob, - const char *extra_args) +QTestState *qtest_init_without_qmp_handshake(const char *extra_args) { QTestState *s; int sock, qmpsock, i; @@ -200,13 +199,12 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, command = g_strdup_printf("exec %s " "-qtest unix:%s,nowait " "-qtest-log %s " - "-chardev socket,path=%s,nowait,id=char0 " - "-mon chardev=char0,mode=control%s " + "-qmp unix:%s,nowait " "-machine accel=qtest " "-display none " "%s", qemu_binary, socket_path, getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null", - qmp_socket_path, use_oob ? ",x-oob=on" : "", + qmp_socket_path, extra_args ?: ""); execlp("/bin/sh", "sh", "-c", command, NULL); exit(1); @@ -241,7 +239,7 @@ QTestState *qtest_init_without_qmp_handshake(bool use_oob, QTestState *qtest_init(const char *extra_args) { - QTestState *s = qtest_init_without_qmp_handshake(false, extra_args); + QTestState *s = qtest_init_without_qmp_handshake(extra_args); /* Read the QMP greeting and then do the handshake */ qtest_qmp_discard_response(s, ""); diff --git a/tests/qmp-test.c b/tests/qmp-test.c index c85a3964d9..a6395363e5 100644 --- a/tests/qmp-test.c +++ b/tests/qmp-test.c @@ -81,7 +81,7 @@ static void test_qmp_protocol(void) QList *capabilities; QTestState *qts; - qts = qtest_init_without_qmp_handshake(false, common_args); + qts = qtest_init_without_qmp_handshake(common_args); /* Test greeting */ resp = qtest_qmp_receive(qts); @@ -146,7 +146,7 @@ static void test_qmp_oob(void) QString *qstr; const char *cmd_id; - qts = qtest_init_without_qmp_handshake(true, common_args); + qts = qtest_init_without_qmp_handshake(common_args); /* Check the greeting message. */ resp = qtest_qmp_receive(qts);
This reverts commit ddee57e0176f6ab53b13c6c97605b62737a8fd7a. Meanwhile, revert one line from fa198ad9bdef to make sure qtest_init_without_qmp_handshake() will only pass in one parameter. Signed-off-by: Peter Xu <peterx@redhat.com> --- tests/libqtest.h | 7 ++----- tests/libqtest.c | 10 ++++------ tests/qmp-test.c | 4 ++-- 3 files changed, 8 insertions(+), 13 deletions(-)