Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
On 09/12/2017 05:45 AM, Thomas Huth wrote: > On 11.09.2017 19:20, Eric Blake wrote: >> Maintaining two layers of libqtest APIs, one that takes an explicit >> QTestState object, and the other that uses the implicit global_qtest, >> is annoying. In the interest of getting rid of global implicit >> state and having less code to maintain, merge: >> qtest_clock_set() >> qtest_clock_step() >> qtest_clock_step_next() >> with their short counterparts. All callers that previously >> used the short form now make it explicit that they are relying on >> global_qtest, and later patches can then clean things up to remove >> the global variable. >> >> @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); >> * >> * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. >> */ >> -int64_t qtest_clock_set(QTestState *s, int64_t val); >> +int64_t clock_set(QTestState *s, int64_t val); > Could we please keep the "qtest" prefix here and rather get rid of the > other ones? Even if it's more to type, I prefer to have a proper prefix > here so that it is clear at the first sight that the functions belong to > the qtest framework. I suppose we can, although it makes more lines that are likely to bump up against 80 columns, and thus slightly more churn to reformat things to keep checkpatch happy. I like the shorter name, because less typing is easier to remember. I'd prefer a second opinion on naming before doing anything about it though - Markus or Paolo, do you have any preference? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
On 11.09.2017 19:20, Eric Blake wrote: > Maintaining two layers of libqtest APIs, one that takes an explicit > QTestState object, and the other that uses the implicit global_qtest, > is annoying. In the interest of getting rid of global implicit > state and having less code to maintain, merge: > qtest_clock_set() > qtest_clock_step() > qtest_clock_step_next() > with their short counterparts. All callers that previously > used the short form now make it explicit that they are relying on > global_qtest, and later patches can then clean things up to remove > the global variable. > > Signed-off-by: Eric Blake> --- > tests/libqtest.h | 50 > tests/libqtest.c | 6 ++-- > tests/e1000e-test.c | 2 +- > tests/fdc-test.c | 4 +-- > tests/ide-test.c | 2 +- > tests/libqos/virtio.c| 8 +++--- > tests/rtc-test.c | 74 > > tests/rtl8139-test.c | 10 +++ > tests/tco-test.c | 22 +++--- > tests/test-arm-mptimer.c | 25 +--- > tests/wdt_ib700-test.c | 12 > 11 files changed, 90 insertions(+), 125 deletions(-) > > diff --git a/tests/libqtest.h b/tests/libqtest.h > index 5651b77d2f..26d5f37bc9 100644 > --- a/tests/libqtest.h > +++ b/tests/libqtest.h > @@ -417,17 +417,17 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, > void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size); > > /** > - * qtest_clock_step_next: > + * clock_step_next: > * @s: #QTestState instance to operate on. > * > * Advance the QEMU_CLOCK_VIRTUAL to the next deadline. > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_step_next(QTestState *s); > +int64_t clock_step_next(QTestState *s); > > /** > - * qtest_clock_step: > + * clock_step: > * @s: QTestState instance to operate on. > * @step: Number of nanoseconds to advance the clock by. > * > @@ -435,10 +435,10 @@ int64_t qtest_clock_step_next(QTestState *s); > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_step(QTestState *s, int64_t step); > +int64_t clock_step(QTestState *s, int64_t step); > > /** > - * qtest_clock_set: > + * clock_set: > * @s: QTestState instance to operate on. > * @val: Nanoseconds value to advance the clock to. > * > @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); > * > * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. > */ > -int64_t qtest_clock_set(QTestState *s, int64_t val); > +int64_t clock_set(QTestState *s, int64_t val); Could we please keep the "qtest" prefix here and rather get rid of the other ones? Even if it's more to type, I prefer to have a proper prefix here so that it is clear at the first sight that the functions belong to the qtest framework. Thomas
[Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()
Maintaining two layers of libqtest APIs, one that takes an explicit QTestState object, and the other that uses the implicit global_qtest, is annoying. In the interest of getting rid of global implicit state and having less code to maintain, merge: qtest_clock_set() qtest_clock_step() qtest_clock_step_next() with their short counterparts. All callers that previously used the short form now make it explicit that they are relying on global_qtest, and later patches can then clean things up to remove the global variable. Signed-off-by: Eric Blake--- tests/libqtest.h | 50 tests/libqtest.c | 6 ++-- tests/e1000e-test.c | 2 +- tests/fdc-test.c | 4 +-- tests/ide-test.c | 2 +- tests/libqos/virtio.c| 8 +++--- tests/rtc-test.c | 74 tests/rtl8139-test.c | 10 +++ tests/tco-test.c | 22 +++--- tests/test-arm-mptimer.c | 25 +--- tests/wdt_ib700-test.c | 12 11 files changed, 90 insertions(+), 125 deletions(-) diff --git a/tests/libqtest.h b/tests/libqtest.h index 5651b77d2f..26d5f37bc9 100644 --- a/tests/libqtest.h +++ b/tests/libqtest.h @@ -417,17 +417,17 @@ void qtest_bufwrite(QTestState *s, uint64_t addr, void qtest_memset(QTestState *s, uint64_t addr, uint8_t patt, size_t size); /** - * qtest_clock_step_next: + * clock_step_next: * @s: #QTestState instance to operate on. * * Advance the QEMU_CLOCK_VIRTUAL to the next deadline. * * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. */ -int64_t qtest_clock_step_next(QTestState *s); +int64_t clock_step_next(QTestState *s); /** - * qtest_clock_step: + * clock_step: * @s: QTestState instance to operate on. * @step: Number of nanoseconds to advance the clock by. * @@ -435,10 +435,10 @@ int64_t qtest_clock_step_next(QTestState *s); * * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. */ -int64_t qtest_clock_step(QTestState *s, int64_t step); +int64_t clock_step(QTestState *s, int64_t step); /** - * qtest_clock_set: + * clock_set: * @s: QTestState instance to operate on. * @val: Nanoseconds value to advance the clock to. * @@ -446,7 +446,7 @@ int64_t qtest_clock_step(QTestState *s, int64_t step); * * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. */ -int64_t qtest_clock_set(QTestState *s, int64_t val); +int64_t clock_set(QTestState *s, int64_t val); /** * qtest_big_endian: @@ -868,44 +868,6 @@ static inline void qmemset(uint64_t addr, uint8_t patt, size_t size) qtest_memset(global_qtest, addr, patt, size); } -/** - * clock_step_next: - * - * Advance the QEMU_CLOCK_VIRTUAL to the next deadline. - * - * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. - */ -static inline int64_t clock_step_next(void) -{ -return qtest_clock_step_next(global_qtest); -} - -/** - * 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); -} - -/** - * clock_set: - * @val: Nanoseconds value to advance the clock to. - * - * Advance the QEMU_CLOCK_VIRTUAL to @val nanoseconds since the VM was launched. - * - * Returns: The current value of the QEMU_CLOCK_VIRTUAL in nanoseconds. - */ -static inline int64_t clock_set(int64_t val) -{ -return qtest_clock_set(global_qtest, val); -} - QDict *qmp_fd_receive(int fd); void qmp_fd_sendv(int fd, const char *fmt, va_list ap); void qmp_fd_send(int fd, const char *fmt, ...); diff --git a/tests/libqtest.c b/tests/libqtest.c index 44c89813ff..9f5f2cb933 100644 --- a/tests/libqtest.c +++ b/tests/libqtest.c @@ -666,19 +666,19 @@ static int64_t qtest_clock_rsp(QTestState *s) return clock; } -int64_t qtest_clock_step_next(QTestState *s) +int64_t clock_step_next(QTestState *s) { qtest_sendf(s, "clock_step\n"); return qtest_clock_rsp(s); } -int64_t qtest_clock_step(QTestState *s, int64_t step) +int64_t clock_step(QTestState *s, int64_t step) { qtest_sendf(s, "clock_step %"PRIi64"\n", step); return qtest_clock_rsp(s); } -int64_t qtest_clock_set(QTestState *s, int64_t val) +int64_t clock_set(QTestState *s, int64_t val) { qtest_sendf(s, "clock_set %"PRIi64"\n", val); return qtest_clock_rsp(s); diff --git a/tests/e1000e-test.c b/tests/e1000e-test.c index 323aabb454..f4ead74f96 100644 --- a/tests/e1000e-test.c +++ b/tests/e1000e-test.c @@ -229,7 +229,7 @@ static void e1000e_wait_isr(e1000e_device *d, uint16_t msg_id) if (qpci_msix_pending(d->pci_dev, msg_id)) { return; } -clock_step(1); +clock_step(global_qtest, 1); } while (g_get_monotonic_time() < end_time);