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

[Eric Blake <eblake@redhat.com>]

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).

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