Re: [PATCH v14 03/11] tests: Add test for QAPI builtin type time

[Tao Xu <tao3.xu@intel.com>]

On 11/14/2019 6:06 AM, Eduardo Habkost wrote:
> On Wed, Nov 13, 2019 at 09:01:29AM +0800, Tao Xu wrote:
>> On 11/13/2019 4:15 AM, Eduardo Habkost wrote:
>>> On Fri, Nov 08, 2019 at 09:05:52AM +0100, Markus Armbruster wrote:
>>>> Tao Xu <tao3.xu@intel.com> writes:
>>>>
>>>>> On 11/7/2019 9:31 PM, Eduardo Habkost wrote:
>>>>>> On Thu, Nov 07, 2019 at 02:24:52PM +0800, Tao Xu wrote:
>>>>>>> On 11/7/2019 4:53 AM, Eduardo Habkost wrote:
>>>>>>>> On Mon, Oct 28, 2019 at 03:52:12PM +0800, Tao Xu wrote:
>>>>>>>>> Add tests for time input such as zero, around limit of precision,
>>>>>>>>> signed upper limit, actual upper limit, beyond limits, time suffixes,
>>>>>>>>> and etc.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>>>>>> ---
>>>>>>>> [...]
>>>>>>>>> +    /* Close to signed upper limit 0x7ffffffffffffc00 (53 msbs set) */
>>>>>>>>> +    qdict = keyval_parse("time1=9223372036854774784," /* 7ffffffffffffc00 */
>>>>>>>>> +                         "time2=9223372036854775295", /* 7ffffffffffffdff */
>>>>>>>>> +                         NULL, &error_abort);
>>>>>>>>> +    v = qobject_input_visitor_new_keyval(QOBJECT(qdict));
>>>>>>>>> +    qobject_unref(qdict);
>>>>>>>>> +    visit_start_struct(v, NULL, NULL, 0, &error_abort);
>>>>>>>>> +    visit_type_time(v, "time1", &time, &error_abort);
>>>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>>>> +    visit_type_time(v, "time2", &time, &error_abort);
>>>>>>>>> +    g_assert_cmphex(time, ==, 0x7ffffffffffffc00);
>>>>>>>>
>>>>>>>> I'm confused by this test case and the one below[1].  Are these
>>>>>>>> known bugs?  Shouldn't we document them as known bugs?
>>>>>>>
>>>>>>> Because do_strtosz() or do_strtomul() actually parse with strtod(), so the
>>>>>>> precision is 53 bits, so in these cases, 7ffffffffffffdff and
>>>>>>> fffffffffffffbff are rounded.
>>>>>>
>>>>>> My questions remain: why isn't this being treated like a bug?
>>>>>>
>>>>> Hi Markus,
>>>>>
>>>>> I am confused about the code here too. Because in do_strtosz(), the
>>>>> upper limit is
>>>>>
>>>>> val * mul >= 0xfffffffffffffc00
>>>>>
>>>>> So some data near 53 bit may be rounded. Is there a bug?
>>>>
>>>> No, but the design is surprising, and the functions lack written
>>>> contracts, except for the do_strtosz() helper, which has one that sucks.
>>>>
>>>> qemu_strtosz() & friends are designed to accept fraction * unit
>>>> multiplier.  Example: 1.5M means 1.5 * 1024 * 1024 with qemu_strtosz()
>>>> and qemu_strtosz_MiB(), and 1.5 * 1000 * 1000 with
>>>> qemu_strtosz_metric().  Whether supporting fractions is a good idea is
>>>> debatable, but it's what we've got.
>>>>
>>>> The implementation limits the numeric part to the precision of double,
>>>> i.e. 53 bits.  "8PiB should be enough for anybody."
>>>>
>>>> Switching it from double to long double raises the limit to the
>>>> precision of long double.  At least 64 bit on common hosts, but hosts
>>>> exist where it's the same 53 bits.  Do we support any such hosts?  If
>>>> yes, then we'd make the precision depend on the host, which feels like a
>>>> bad idea.
>>>>
>>>> A possible alternative is to parse the numeric part both as a double and
>>>> as a 64 bit unsigned integer, then use whatever consumes more
>>>> characters.  This enables providing full 64 bits unless you actually use
>>>> a fraction.
>>>>
>>>
>>> This sounds like the right thing to do, if user input is an
>>> integer and the code in the other end is consuming an integer.
>>>
>>>
>>>> As far as I remember, the only problem we've ever had with the 53 bits
>>>> limit is developer confusion :)
>>>>
>>>
>>> Developer confusion, I can deal with.  However, exposing this
>>> behavior on external interfaces is a bug to me.
>>>
>>> I don't know how serious the bug is because I don't know which
>>> interfaces are affected by it.  Do we have a list?
>>>
>>>> Patches welcome.
>>>
>>> My first goal is to get the maintainers of that code to recognize
>>> it as a bug.  Then I hope this will motivate somebody else to fix
>>> it.  :)
>>>
>>
>> Hi Eduardo,
>>
>> If it is a bug, could the fix patch merged during rc1-rc3? Because I made 2
>> patches, and I want to submit before HMAT (HMAT patches is big, so submit
>> together may be slow).
> 
> Even if I convince other maintainers it is a bug, I don't think
> it is serious enough to require a fix in QEMU 4.2.  I suggest
> finishing the ongoing HMAT work first, and worry about this issue
> later.
> 
> Or, if you really prefer to address it before HMAT, it's OK to
> make the next version of the HMAT series depend on a series
> that's not merged yet.  Just make this explicit in the series
> cover letter (publishing a git branch to help review and testing
> is also appreciated).
> 
OK I will submit them together.