Re: [Qemu-devel] [RFC v2 4/4] Revert "tests: Add parameter to qtest_init_without_qmp_handshake"

[Peter Xu <peterx@redhat.com>]

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.

-- 
Peter Xu