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

[Thomas Huth <thuth@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

On 12.09.2017 15:35, Eric Blake wrote:
> 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?

IMHO you should at least keep the qtest prefix in patch 33/38 to avoid
confusion with the system definitions that have the same names (see "man
2 outb" for example).

 Thomas


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]