diff mbox series

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

Message ID 20191028075220.25673-4-tao3.xu@intel.com
State New
Headers show
Series Build ACPI Heterogeneous Memory Attribute Table (HMAT) | expand

Commit Message

Tao Xu Oct. 28, 2019, 7:52 a.m. UTC
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>
---

Changes in v14:
    - Drop time unit picosecond (Eric)
---
 tests/test-keyval.c                | 122 +++++++++++++++++++++++++++++
 tests/test-qobject-input-visitor.c |  29 +++++++
 2 files changed, 151 insertions(+)

Comments

Eduardo Habkost Nov. 6, 2019, 8:53 p.m. UTC | #1
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?

> +    visit_check_struct(v, &error_abort);
> +    visit_end_struct(v, NULL);
> +    visit_free(v);
> +
> +    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
> +    qdict = keyval_parse("time1=18446744073709549568," /* fffffffffffff800 */
> +                         "time2=18446744073709550591", /* fffffffffffffbff */
> +                         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, ==, 0xfffffffffffff800);
> +    visit_type_time(v, "time2", &time, &error_abort);
> +    g_assert_cmphex(time, ==, 0xfffffffffffff800);

[1]

> +    visit_check_struct(v, &error_abort);
> +    visit_end_struct(v, NULL);
> +    visit_free(v);
[...]
Tao Xu Nov. 7, 2019, 6:24 a.m. UTC | #2
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.
> 
>> +    visit_check_struct(v, &error_abort);
>> +    visit_end_struct(v, NULL);
>> +    visit_free(v);
>> +
>> +    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
>> +    qdict = keyval_parse("time1=18446744073709549568," /* fffffffffffff800 */
>> +                         "time2=18446744073709550591", /* fffffffffffffbff */
>> +                         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, ==, 0xfffffffffffff800);
>> +    visit_type_time(v, "time2", &time, &error_abort);
>> +    g_assert_cmphex(time, ==, 0xfffffffffffff800);
> 
> [1]
> 
>> +    visit_check_struct(v, &error_abort);
>> +    visit_end_struct(v, NULL);
>> +    visit_free(v);
> [...]
>
Eduardo Habkost Nov. 7, 2019, 1:31 p.m. UTC | #3
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?
Tao Xu Nov. 8, 2019, 5:25 a.m. UTC | #4
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?
Markus Armbruster Nov. 8, 2019, 8:05 a.m. UTC | #5
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.

As far as I remember, the only problem we've ever had with the 53 bits
limit is developer confusion :)

Patches welcome.
Igor Mammedov Nov. 8, 2019, 8:41 a.m. UTC | #6
On Fri, 08 Nov 2019 09:05:52 +0100
Markus Armbruster <armbru@redhat.com> 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.
> 
> As far as I remember, the only problem we've ever had with the 53 bits
> limit is developer confusion :)

On CLI, we could (a)use full 64bit (-1) lat/bw to mark unreachable nodes.
Also it would be more consistent for both QMP and CLI to be able
handle the same range. This way what was configured over QMP could be
also configured using CLI.

> Patches welcome.
Tao Xu Nov. 11, 2019, 3:12 a.m. UTC | #7
On 11/8/2019 4:41 PM, Igor Mammedov wrote:
> On Fri, 08 Nov 2019 09:05:52 +0100
> Markus Armbruster <armbru@redhat.com> 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.
>>
>> As far as I remember, the only problem we've ever had with the 53 bits
>> limit is developer confusion :)
> 
> On CLI, we could (a)use full 64bit (-1) lat/bw to mark unreachable nodes.
> Also it would be more consistent for both QMP and CLI to be able
> handle the same range. This way what was configured over QMP could be
> also configured using CLI.
> 

OK. I will add a new patch to do this. Next version we will submit
separated patches for QAPI builtin type changes.
Igor Mammedov Nov. 11, 2019, 10:02 a.m. UTC | #8
On Mon, 11 Nov 2019 11:12:28 +0800
Tao Xu <tao3.xu@intel.com> wrote:

> On 11/8/2019 4:41 PM, Igor Mammedov wrote:
> > On Fri, 08 Nov 2019 09:05:52 +0100
> > Markus Armbruster <armbru@redhat.com> 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.
> >>
> >> As far as I remember, the only problem we've ever had with the 53 bits
> >> limit is developer confusion :)  
> > 
> > On CLI, we could (a)use full 64bit (-1) lat/bw to mark unreachable nodes.
> > Also it would be more consistent for both QMP and CLI to be able
> > handle the same range. This way what was configured over QMP could be
> > also configured using CLI.
> >   
> 
> OK. I will add a new patch to do this. Next version we will submit
> separated patches for QAPI builtin type changes.

Since you've got rid of magic handling from CLI parsing, it's not
must have feauture but it would be nice to have and probably could
be done on top of HMAT patches.
Eduardo Habkost Nov. 12, 2019, 8:15 p.m. UTC | #9
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.  :)
Tao Xu Nov. 13, 2019, 1:01 a.m. UTC | #10
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).

Tao
Eduardo Habkost Nov. 13, 2019, 10:06 p.m. UTC | #11
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).
Tao Xu Nov. 14, 2019, 12:51 a.m. UTC | #12
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.
diff mbox series

Patch

diff --git a/tests/test-keyval.c b/tests/test-keyval.c
index 09b0ae3c68..28e2e34084 100644
--- a/tests/test-keyval.c
+++ b/tests/test-keyval.c
@@ -490,6 +490,127 @@  static void test_keyval_visit_size(void)
     visit_free(v);
 }
 
+static void test_keyval_visit_time(void)
+{
+    Error *err = NULL;
+    Visitor *v;
+    QDict *qdict;
+    uint64_t time;
+
+    /* Lower limit zero */
+    qdict = keyval_parse("time1=0", 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_cmpuint(time, ==, 0);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Around limit of precision: 2^53-1, 2^53, 2^53+1 */
+    qdict = keyval_parse("time1=9007199254740991,"
+                         "time2=9007199254740992,"
+                         "time3=9007199254740993",
+                         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, ==, 0x1fffffffffffff);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x20000000000000);
+    visit_type_time(v, "time3", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0x20000000000000);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* 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);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Close to actual upper limit 0xfffffffffffff800 (53 msbs set) */
+    qdict = keyval_parse("time1=18446744073709549568," /* fffffffffffff800 */
+                         "time2=18446744073709550591", /* fffffffffffffbff */
+                         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, ==, 0xfffffffffffff800);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmphex(time, ==, 0xfffffffffffff800);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Beyond limits */
+    qdict = keyval_parse("time1=-1,"
+                         "time2=18446744073709550592", /* fffffffffffffc00 */
+                         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, &err);
+    error_free_or_abort(&err);
+    visit_type_time(v, "time2", &time, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Suffixes */
+    qdict = keyval_parse("time1=2ns,time2=3.4us,time3=5ms,time4=600s",
+                         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_cmpuint(time, ==, 2);
+    visit_type_time(v, "time2", &time, &error_abort);
+    g_assert_cmpuint(time, ==, 3400);
+    visit_type_time(v, "time3", &time, &error_abort);
+    g_assert_cmphex(time, ==, 5 * 1000 * 1000);
+    visit_type_time(v, "time4", &time, &error_abort);
+    g_assert_cmphex(time, ==, 600 * 1000000000LL);
+    visit_check_struct(v, &error_abort);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Beyond limit with suffix */
+    qdict = keyval_parse("time1=1844674407370955s", 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, &err);
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+
+    /* Trailing crap */
+    qdict = keyval_parse("time1=89ks,time2=ns", 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, &err);
+    error_free_or_abort(&err);
+    visit_type_time(v, "time2", &time, &err);;
+    error_free_or_abort(&err);
+    visit_end_struct(v, NULL);
+    visit_free(v);
+}
+
 static void test_keyval_visit_dict(void)
 {
     Error *err = NULL;
@@ -678,6 +799,7 @@  int main(int argc, char *argv[])
     g_test_add_func("/keyval/visit/bool", test_keyval_visit_bool);
     g_test_add_func("/keyval/visit/number", test_keyval_visit_number);
     g_test_add_func("/keyval/visit/size", test_keyval_visit_size);
+    g_test_add_func("/keyval/visit/time", test_keyval_visit_time);
     g_test_add_func("/keyval/visit/dict", test_keyval_visit_dict);
     g_test_add_func("/keyval/visit/list", test_keyval_visit_list);
     g_test_add_func("/keyval/visit/optional", test_keyval_visit_optional);
diff --git a/tests/test-qobject-input-visitor.c b/tests/test-qobject-input-visitor.c
index 6bacabf063..55138042b8 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -366,6 +366,31 @@  static void test_visitor_in_size_str_fail(TestInputVisitorData *data,
     error_free_or_abort(&err);
 }
 
+static void test_visitor_in_time_str_keyval(TestInputVisitorData *data,
+                                            const void *unused)
+{
+    uint64_t res, value = 265 * 1000 * 1000;
+    Visitor *v;
+
+    v = visitor_input_test_init_full(data, true, "\"265ms\"");
+
+    visit_type_time(v, NULL, &res, &error_abort);
+    g_assert_cmpfloat(res, ==, value);
+}
+
+static void test_visitor_in_time_str_fail(TestInputVisitorData *data,
+                                          const void *unused)
+{
+    uint64_t res = 0;
+    Visitor *v;
+    Error *err = NULL;
+
+    v = visitor_input_test_init(data, "\"265ms\"");
+
+    visit_type_time(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+}
+
 static void test_visitor_in_string(TestInputVisitorData *data,
                                    const void *unused)
 {
@@ -1311,6 +1336,10 @@  int main(int argc, char **argv)
                            NULL, test_visitor_in_size_str_keyval);
     input_visitor_test_add("/visitor/input/size_str_fail",
                            NULL, test_visitor_in_size_str_fail);
+    input_visitor_test_add("/visitor/input/time_str_keyval",
+                           NULL, test_visitor_in_time_str_keyval);
+    input_visitor_test_add("/visitor/input/time_str_fail",
+                           NULL, test_visitor_in_time_str_fail);
     input_visitor_test_add("/visitor/input/string",
                            NULL, test_visitor_in_string);
     input_visitor_test_add("/visitor/input/enum",