[PATCH] qtests: avoid printing comments before g_test_init()

[PATCH] qtests: avoid printing comments before g_test_init()

[Daniel P. Berrangé]

The TAP protocol version line must be the first thing printed on
stdout. The migration test failed that requirement in certain
scenarios:

  # Skipping test: Userfault not available (builtdtime)
  TAP version 13
  # random seed: R02Sc120c807f11053eb90bfea845ba1e368
  1..32
  # Start of x86_64 tests
  # Start of migration tests
  ....

The TAP version is printed by g_test_init(), so we need to make
sure that any methods which print are run after that.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 tests/qtest/migration-test.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 75d4f1d4a9..3b615b0da9 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2462,14 +2462,18 @@ static bool kvm_dirty_ring_supported(void)
 
 int main(int argc, char **argv)
 {
-    const bool has_kvm = qtest_has_accel("kvm");
-    const bool has_uffd = ufd_version_check();
-    const char *arch = qtest_get_arch();
+    bool has_kvm;
+    bool has_uffd;
+    const char *arch;
     g_autoptr(GError) err = NULL;
     int ret;
 
     g_test_init(&argc, &argv, NULL);
 
+    has_kvm = qtest_has_accel("kvm");
+    has_uffd = ufd_version_check();
+    arch = qtest_get_arch();
+
     /*
      * On ppc64, the test only works with kvm-hv, but not with kvm-pr and TCG
      * is touchy due to race conditions on dirty bits (especially on PPC for
-- 
2.39.2

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Peter Maydell]

On Fri, 17 Mar 2023 at 17:06, Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> The TAP protocol version line must be the first thing printed on
> stdout. The migration test failed that requirement in certain
> scenarios:
>
>   # Skipping test: Userfault not available (builtdtime)
>   TAP version 13
>   # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>   1..32
>   # Start of x86_64 tests
>   # Start of migration tests
>   ....
>
> The TAP version is printed by g_test_init(), so we need to make
> sure that any methods which print are run after that.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Yep, that's exactly the same as the patch I wrote but hadn't
quite got as far as sending :-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Alex Bennée]

Daniel P. Berrangé <berrange@redhat.com> writes:

> The TAP protocol version line must be the first thing printed on
> stdout. The migration test failed that requirement in certain
> scenarios:
>
>   # Skipping test: Userfault not available (builtdtime)
>   TAP version 13
>   # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>   1..32
>   # Start of x86_64 tests
>   # Start of migration tests
>   ....
>
> The TAP version is printed by g_test_init(), so we need to make
> sure that any methods which print are run after that.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Queued to pr/170323-for-8.0-1, thanks.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Thomas Huth]

On 17/03/2023 18.05, Daniel P. Berrangé wrote:
> The TAP protocol version line must be the first thing printed on
> stdout. The migration test failed that requirement in certain
> scenarios:
> 
>    # Skipping test: Userfault not available (builtdtime)
>    TAP version 13
>    # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>    1..32
>    # Start of x86_64 tests
>    # Start of migration tests
>    ....
> 
> The TAP version is printed by g_test_init(), so we need to make
> sure that any methods which print are run after that.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   tests/qtest/migration-test.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> The TAP protocol version line must be the first thing printed on
> stdout. The migration test failed that requirement in certain
> scenarios:
>
>   # Skipping test: Userfault not available (builtdtime)
>   TAP version 13
>   # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>   1..32
>   # Start of x86_64 tests
>   # Start of migration tests
>   ....
>
> The TAP version is printed by g_test_init(), so we need to make
> sure that any methods which print are run after that.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---

Reviewed-by: Juan Quintela <quintela@redhat.com>

> -    const bool has_kvm = qtest_has_accel("kvm");
> -    const bool has_uffd = ufd_version_check();
> -    const char *arch = qtest_get_arch();
> +    bool has_kvm;
> +    bool has_uffd;
> +    const char *arch;

Why don't you move also the declarations of the variables?
I think that one of the biggest troubles of C is variables that are not
initialized.

All compilers that we support are C99 or later, so we can do that (and
we already do in lot of places.)

And yeap, I know that CodingStyle says otherwise, but I think that what
is wrong is CodingStyle.

https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03836.html

Later, Juan.

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Thomas Huth]

On 23/03/2023 19.31, Juan Quintela wrote:
> Daniel P. Berrangé <berrange@redhat.com> wrote:
>> The TAP protocol version line must be the first thing printed on
>> stdout. The migration test failed that requirement in certain
>> scenarios:
>>
>>    # Skipping test: Userfault not available (builtdtime)
>>    TAP version 13
>>    # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>>    1..32
>>    # Start of x86_64 tests
>>    # Start of migration tests
>>    ....
>>
>> The TAP version is printed by g_test_init(), so we need to make
>> sure that any methods which print are run after that.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
>> -    const bool has_kvm = qtest_has_accel("kvm");
>> -    const bool has_uffd = ufd_version_check();
>> -    const char *arch = qtest_get_arch();
>> +    bool has_kvm;
>> +    bool has_uffd;
>> +    const char *arch;
> 
> Why don't you move also the declarations of the variables?
> I think that one of the biggest troubles of C is variables that are not
> initialized.
> 
> All compilers that we support are C99 or later, so we can do that (and
> we already do in lot of places.)

I think the coding style has been created before we switched to -std=gnu99 
for compiling QEMU, so a lot of GCCs were still using C89 by default?

> And yeap, I know that CodingStyle says otherwise, but I think that what
> is wrong is CodingStyle.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03836.html

Please use proper prefixes in the subject when sending patches 
("docs/devel:" here), otherwise your patches might not get the right 
attention (at least on my side, it was filtered away as a patch that was 
relevant to me) - and also put some recent contributors on CC:

  Thanks,
   Thomas

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Juan Quintela]

Thomas Huth <thuth@redhat.com> wrote:
> On 23/03/2023 19.31, Juan Quintela wrote:
>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>>> The TAP protocol version line must be the first thing printed on
>>> stdout. The migration test failed that requirement in certain
>>> scenarios:
>>>
>>>    # Skipping test: Userfault not available (builtdtime)
>>>    TAP version 13
>>>    # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>>>    1..32
>>>    # Start of x86_64 tests
>>>    # Start of migration tests
>>>    ....
>>>
>>> The TAP version is printed by g_test_init(), so we need to make
>>> sure that any methods which print are run after that.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> 
>>> -    const bool has_kvm = qtest_has_accel("kvm");
>>> -    const bool has_uffd = ufd_version_check();
>>> -    const char *arch = qtest_get_arch();
>>> +    bool has_kvm;
>>> +    bool has_uffd;
>>> +    const char *arch;
>> Why don't you move also the declarations of the variables?
>> I think that one of the biggest troubles of C is variables that are not
>> initialized.
>> All compilers that we support are C99 or later, so we can do that
>> (and
>> we already do in lot of places.)
>
> I think the coding style has been created before we switched to
> -std=gnu99 for compiling QEMU, so a lot of GCCs were still using C89
> by default?

Yes, that is the actitude.

I got sick when I see new code that still does:

char *foo = (char *)malloc(...);

It is is C89, it has been enough to know that it is not needed.

And yes, that particular one is not used in qemu anymore, but:

void *opaque;

....

Foo *foo = (Foo *)opaque;

Is still introduced in new code, and it is not needed since C89.

>> And yeap, I know that CodingStyle says otherwise, but I think that what
>> is wrong is CodingStyle.
>> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03836.html
>
> Please use proper prefixes in the subject when sending patches
> ("docs/devel:" here), otherwise your patches might not get the right
> attention (at least on my side, it was filtered away as a patch that
> was relevant to me) - and also put some recent contributors on CC:

I didn't knew the docs/devel preffix.

About the CC'd, I expected that git-publish be good enough at doing
that, but it appears not.

Anyways, thanks.

Later, Juan.

Re: [PATCH] qtests: avoid printing comments before g_test_init()

[Alex Bennée]

Juan Quintela <quintela@redhat.com> writes:

> Thomas Huth <thuth@redhat.com> wrote:
>> On 23/03/2023 19.31, Juan Quintela wrote:
>>> Daniel P. Berrangé <berrange@redhat.com> wrote:
>>>> The TAP protocol version line must be the first thing printed on
>>>> stdout. The migration test failed that requirement in certain
>>>> scenarios:
>>>>
>>>>    # Skipping test: Userfault not available (builtdtime)
>>>>    TAP version 13
>>>>    # random seed: R02Sc120c807f11053eb90bfea845ba1e368
>>>>    1..32
>>>>    # Start of x86_64 tests
>>>>    # Start of migration tests
>>>>    ....
>>>>
>>>> The TAP version is printed by g_test_init(), so we need to make
>>>> sure that any methods which print are run after that.
>>>>
>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>> ---
>>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>>> 
>>>> -    const bool has_kvm = qtest_has_accel("kvm");
>>>> -    const bool has_uffd = ufd_version_check();
>>>> -    const char *arch = qtest_get_arch();
>>>> +    bool has_kvm;
>>>> +    bool has_uffd;
>>>> +    const char *arch;
>>> Why don't you move also the declarations of the variables?
>>> I think that one of the biggest troubles of C is variables that are not
>>> initialized.
>>> All compilers that we support are C99 or later, so we can do that
>>> (and
>>> we already do in lot of places.)
>>
>> I think the coding style has been created before we switched to
>> -std=gnu99 for compiling QEMU, so a lot of GCCs were still using C89
>> by default?
>
> Yes, that is the actitude.
>
> I got sick when I see new code that still does:
>
> char *foo = (char *)malloc(...);
>
> It is is C89, it has been enough to know that it is not needed.
>
> And yes, that particular one is not used in qemu anymore, but:
>
> void *opaque;
>
> ....
>
> Foo *foo = (Foo *)opaque;
>
> Is still introduced in new code, and it is not needed since C89.
>
>>> And yeap, I know that CodingStyle says otherwise, but I think that what
>>> is wrong is CodingStyle.
>>> https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg03836.html
>>
>> Please use proper prefixes in the subject when sending patches
>> ("docs/devel:" here), otherwise your patches might not get the right
>> attention (at least on my side, it was filtered away as a patch that
>> was relevant to me) - and also put some recent contributors on CC:
>
> I didn't knew the docs/devel preffix.
>
> About the CC'd, I expected that git-publish be good enough at doing
> that, but it appears not.

I've just sent:

  Subject: [RFC PATCH] MAINTAINERS: add a section for policy documents
  Date: Fri, 24 Mar 2023 17:38:36 +0000
  Message-Id: <20230324173836.1821275-1-alex.bennee@linaro.org>

to collect willing victims^H^H^H^H^H^H volunteers who want to track
project related discussions like this. 

>
> Anyways, thanks.
>
> Later, Juan.


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro