| Message ID | 20230317170553.592707-1-berrange@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qtests: avoid printing comments before g_test_init() | expand |
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
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.
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>
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.
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
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.
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.
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
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(-)