Re: [Qemu-devel] [PATCH v2 5/6] tests/libqtest: Move global_test wrapper function into a separate header
On 04/09/2019 21.09, Eric Blake wrote:
> On 9/4/19 1:51 PM, Stefan Hajnoczi wrote:
>> On Wed, Sep 04, 2019 at 03:00:46PM +0200, Thomas Huth wrote:
>>> diff --git a/tests/libqtest-single.h b/tests/libqtest-single.h
>>> new file mode 100644
>>> index 00..49259558a5
>>> --- /dev/null
>>> +++ b/tests/libqtest-single.h
>
>>> +static inline QTestState *qtest_start(const char *args)
>>> +{
>>> +global_qtest = qtest_init(args);
>>
>> Where are global_qtest and qtest_init() declared? I would expect
>> compilation to fail if a .c file included just "libqtest-single.h".
>
> In patch 5, "libqtest.h" declares global_qtest, then includes
> "libqtest-single.h"; no file includes it standalone. Then in patch 6,
> the roles are swapped; "libqtest-single.h" declares global_qtest and
> includes "libqtest.h". But yes, the commit message could do better in
> explaining this.
Right. Let me add something like this to the patch description here:
"The new header is only included from libqtest.h for now, so that there
is no difference to the users of libqtest.h yet. In the next patch, we
will switch this, so that the users of the global_qtest-related
functions will be using libqtest-single.h directly and libqtest.h
becomes completely independent of this."
Thomas
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 5/6] tests/libqtest: Move global_test wrapper function into a separate header
On 9/4/19 1:51 PM, Stefan Hajnoczi wrote:
> On Wed, Sep 04, 2019 at 03:00:46PM +0200, Thomas Huth wrote:
>> diff --git a/tests/libqtest-single.h b/tests/libqtest-single.h
>> new file mode 100644
>> index 00..49259558a5
>> --- /dev/null
>> +++ b/tests/libqtest-single.h
>> +static inline QTestState *qtest_start(const char *args)
>> +{
>> +global_qtest = qtest_init(args);
>
> Where are global_qtest and qtest_init() declared? I would expect
> compilation to fail if a .c file included just "libqtest-single.h".
In patch 5, "libqtest.h" declares global_qtest, then includes
"libqtest-single.h"; no file includes it standalone. Then in patch 6,
the roles are swapped; "libqtest-single.h" declares global_qtest and
includes "libqtest.h". But yes, the commit message could do better in
explaining this.
> Missing #include?
>
> If this header is not supposed to be included by .c files, please
> include at least a comment or use a magic #define + #ifdef to prevent
> inclusion.
The condition is transient for one patch; but calling it out better in
the commit message and/or as a comment (especially since the #include
"libqtest-single.h" is not in the normal #includes up front) would help.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v2 5/6] tests/libqtest: Move global_test wrapper function into a separate header
On Wed, Sep 04, 2019 at 03:00:46PM +0200, Thomas Huth wrote:
> diff --git a/tests/libqtest-single.h b/tests/libqtest-single.h
> new file mode 100644
> index 00..49259558a5
> --- /dev/null
> +++ b/tests/libqtest-single.h
> @@ -0,0 +1,311 @@
> +/*
> + * QTest - wrappers for test with single QEMU instances
> + *
> + * Copyright IBM, Corp. 2012
> + * Copyright Red Hat, Inc. 2012
> + * Copyright SUSE LINUX Products GmbH 2013
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef LIBQTEST_SINGLE_H
> +#define LIBQTEST_SINGLE_H
> +
> +/**
> + * qtest_start:
> + * @args: other arguments to pass to QEMU
> + *
> + * Start QEMU and assign the resulting #QTestState to a global variable.
> + * The global variable is used by "shortcut" functions documented below.
> + *
> + * Returns: #QTestState instance.
> + */
> +static inline QTestState *qtest_start(const char *args)
> +{
> +global_qtest = qtest_init(args);
Where are global_qtest and qtest_init() declared? I would expect
compilation to fail if a .c file included just "libqtest-single.h".
Missing #include?
If this header is not supposed to be included by .c files, please
include at least a comment or use a magic #define + #ifdef to prevent
inclusion.
signature.asc
Description: PGP signature
Re: [Qemu-devel] [PATCH v2 5/6] tests/libqtest: Move global_test wrapper function into a separate header
On 9/4/19 8:00 AM, Thomas Huth wrote:
> We want libqtest.h to become completely independen from global_qtest
independent
> (so that the wrapper functions are not used by accident anymore). As
> a first step, move the wrapper functions into a separate header file.
>
> Signed-off-by: Thomas Huth
> ---
> MAINTAINERS | 2 +-
> tests/libqtest-single.h | 311
> tests/libqtest.c| 11 --
> tests/libqtest.h| 287 +---
> 4 files changed, 313 insertions(+), 298 deletions(-)
> create mode 100644 tests/libqtest-single.h
>
> +/**
> + * qmp:
> + * @fmt...: QMP message to send to qemu, formatted like
> + * qobject_from_jsonf_nofail(). See parse_escape() for what's
> + * supported after '%'.
> + *
> + * Sends a QMP message to QEMU and returns the response.
> + */
> +GCC_FMT_ATTR(1, 2)
> +static inline QDict *qmp(const char *fmt, ...)
> +{
I'm a bit surprised gcc doesn't complain about inlining a va_arg
function, but since it works, I'm fine with the approach.
> +++ b/tests/libqtest.c
> @@ -1106,17 +1106,6 @@ void qtest_memset(QTestState *s, uint64_t addr,
> uint8_t pattern, size_t size)
> qtest_rsp(s, 0);
> }
>
> -QDict *qmp(const char *fmt, ...)
> -{
> -va_list ap;
> -QDict *response;
> -
> -va_start(ap, fmt);
> -response = qtest_vqmp(global_qtest, fmt, ap);
> -va_end(ap);
> -return response;
> -}
> -
Nice - we've reduced the use of global_qtest to just a single optional
header!
> +++ b/tests/libqtest.h
> -/**
> - * clock_step:
> - * @step: Number of nanoseconds to advance the clock by.
> - *
> - * Advance the QEMU_CLOCK_VIRTUAL by @step nanoseconds.
> - *
> - * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds.
> - */
> -static inline int64_t clock_step(int64_t step)
> -{
> -return qtest_clock_step(global_qtest, step);
> -}
> +#include "libqtest-single.h"
Well, almost. I guess this commit is the code motion, and the next one
actually fixes clients to track the rename, so this #include is
temporary (I hope). It's just churn to put a TODO comment in this
commit that would get removed in the next. So,
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
