qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PULL 092/103] qapi: make string output visitor parse i


From: Michael S. Tsirkin
Subject: Re: [Qemu-devel] [PULL 092/103] qapi: make string output visitor parse int list
Date: Wed, 18 Jun 2014 18:02:09 +0300

On Tue, Jun 17, 2014 at 03:45:29PM -0600, Eric Blake wrote:
> On 06/17/2014 11:41 AM, Michael S. Tsirkin wrote:
> > From: Hu Tao <address@hidden>
> > 
> > Signed-off-by: Hu Tao <address@hidden>
> > Acked-by: Michael S. Tsirkin <address@hidden>
> > Tested-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Michael S. Tsirkin <address@hidden>
> > 
> > MST: split up patch
> > ---
> >  qapi/string-output-visitor.c       | 229 
> > +++++++++++++++++++++++++++++++++++--
> >  tests/test-string-output-visitor.c |  38 +++++-
> >  2 files changed, 255 insertions(+), 12 deletions(-)
> > 
> 
> > +
> > +    l = sov->ranges;
> > +    while (l) {
> > +        Range *r = l->data;
> > +        format_string(sov, r, l->next != NULL, false);
> > +        l = l->next;
> > +    }
> >  
> >      if (sov->human) {
> > -        out = g_strdup_printf("%lld (%#llx)", (long long) *obj, (long 
> > long) *obj);
> > -    } else {
> > -        out = g_strdup_printf("%lld", (long long) *obj);
> > +        l = sov->ranges;
> > +        g_string_append(sov->string, " (");
> > +        while (l) {
> > +            Range *r = l->data;
> > +            format_string(sov, r, l->next != NULL, false);
> 
> Am I reading this correctly that in human mode, you are creating the string:
> 
> 16-31 (16-31)
> 
> instead of
> 
> 16-17 (10-1f)
> 
> because you forgot to pass 'true' as the human parameter on one of the
> two calls to format_string?  Also, this is a worsening of quality; the
> old code would produce
> 
> 16 (0x10)
> 
> to make it obvious which number was hex.
> 
> > +static void test_visitor_out_intList(TestOutputVisitorData *data,
> > +                                     const void *unused)
> > +{
> > +    int64_t value[] = {0, 1, 9, 10, 16, 15, 14,
> > +        3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
> 
> No test of negative numbers?
> 
> > +    str = string_output_get_string(data->sov);
> > +    g_assert(str != NULL);
> > +    g_assert_cmpstr(str, ==,
> > +        "0-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807");
> 
> Shouldn't you also test the human output?
> 
> Probably worth a followup patch.

Hu Tao, could you add tests please?
I have fixed up other issues.

> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 





reply via email to

[Prev in Thread] Current Thread [Next in Thread]