Re: [Qemu-block] [PATCH v7 31/38] libqtest: Merge qtest_clock_*() with clock_*()

2017-09-12 Thread Eric Blake 
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_*()

2017-09-12 Thread Thomas Huth 
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_*()

2017-09-11 Thread Eric Blake 
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);