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 <pet...@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.
--
Peter Xu
