Patchwork [2/2] target-i386: use visit_type_unit_suffixed_int() to parse tsc_freq property value

login
register
mail settings
Submitter Igor Mammedov
Date Dec. 6, 2012, 9:12 p.m.
Message ID <1354828325-16568-3-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/204321/
State New
Headers show

Comments

Igor Mammedov - Dec. 6, 2012, 9:12 p.m.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
  v2:
   - replace visit_type_freq() with visit_type_unit_suffixed_int()
     in x86_cpuid_set_tsc_freq()
---
 target-i386/cpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Michael Roth - Dec. 6, 2012, 10:24 p.m.
On Thu, Dec 06, 2012 at 10:12:05PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Reviewed-by: Michael Roth <mdroth@linux.vnet.ibm.com>

> ---
>   v2:
>    - replace visit_type_freq() with visit_type_unit_suffixed_int()
>      in x86_cpuid_set_tsc_freq()
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..b7f0aba 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = INT64_MAX;
>      int64_t value;
> 
> -    visit_type_int(v, &value, name, errp);
> +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
>      if (error_is_set(errp)) {
>          return;
>      }
> -- 
> 1.7.11.7
>
Eduardo Habkost - Dec. 7, 2012, 11:57 a.m.
On Thu, Dec 06, 2012 at 10:12:05PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v2:
>    - replace visit_type_freq() with visit_type_unit_suffixed_int()
>      in x86_cpuid_set_tsc_freq()

visit_type_unit_suffixed_int() matches the behavior of the current
tsc_freq parsing code on cpu_x86_parse_featurestr(), so:

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..b7f0aba 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = INT64_MAX;
>      int64_t value;
>  
> -    visit_type_int(v, &value, name, errp);
> +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
>      if (error_is_set(errp)) {
>          return;
>      }
> -- 
> 1.7.11.7
>
Andreas Färber - Dec. 7, 2012, 7 p.m.
Am 06.12.2012 22:12, schrieb Igor Mammedov:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>   v2:
>    - replace visit_type_freq() with visit_type_unit_suffixed_int()
>      in x86_cpuid_set_tsc_freq()
> ---
>  target-i386/cpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index c6c2ca0..b7f0aba 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
>      const int64_t max = INT64_MAX;
>      int64_t value;
>  
> -    visit_type_int(v, &value, name, errp);
> +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
>      if (error_is_set(errp)) {
>          return;
>      }

This trivial usage is fine obviously. But since this series set out to
make things more generic I am missing at least one use case for 1024.
Does nothing like that exist in qdev-properties.c or so already?

Andreas
Eduardo Habkost - Dec. 7, 2012, 8:09 p.m.
On Fri, Dec 07, 2012 at 08:00:09PM +0100, Andreas Färber wrote:
> Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v2:
> >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> >      in x86_cpuid_set_tsc_freq()
> > ---
> >  target-i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c6c2ca0..b7f0aba 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      const int64_t max = INT64_MAX;
> >      int64_t value;
> >  
> > -    visit_type_int(v, &value, name, errp);
> > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> >      if (error_is_set(errp)) {
> >          return;
> >      }
> 
> This trivial usage is fine obviously. But since this series set out to
> make things more generic I am missing at least one use case for 1024.
> Does nothing like that exist in qdev-properties.c or so already?

cutils.c has:

int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix)
{
    return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
}

$ git grep -w strtosz_suffix
[...]
qapi/opts-visitor.c:    val = strtosz_suffix(opt->str ? opt->str : "", &endptr,
qemu-img.c:        sval = strtosz_suffix(argv[optind++], &end, STRTOSZ_DEFSUFFIX_B);
qemu-img.c:            sval = strtosz_suffix(optarg, &end, STRTOSZ_DEFSUFFIX_B);

The opts-visitor.c match, in turn, is inside opts_type_size(), that's the
->type_size method of OptsVisitor. There are many 'size' elements inside
qapi-schema.json.

I don't see any code using visit_type_size() directly, but I see two users of
type 'size' on qapi-schema.json: NetdevTapOptions and NetdevDumpOptions.

I didn't know that we already had a visitor method using the suffixed-int
parsing code. Should we change the visit_type_size() code to be to use use the
new generic ->type_suffixed_int method and kill ->type_size?
Igor Mammedov - Dec. 10, 2012, 4:13 p.m.
On Fri, 7 Dec 2012 18:09:06 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Dec 07, 2012 at 08:00:09PM +0100, Andreas Färber wrote:
> > Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >   v2:
> > >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> > >      in x86_cpuid_set_tsc_freq()
> > > ---
> > >  target-i386/cpu.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > index c6c2ca0..b7f0aba 100644
> > > --- a/target-i386/cpu.c
> > > +++ b/target-i386/cpu.c
> > > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> > > Visitor *v, void *opaque, const int64_t max = INT64_MAX;
> > >      int64_t value;
> > >  
> > > -    visit_type_int(v, &value, name, errp);
> > > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> > >      if (error_is_set(errp)) {
> > >          return;
> > >      }
> > 
> > This trivial usage is fine obviously. But since this series set out to
> > make things more generic I am missing at least one use case for 1024.
> > Does nothing like that exist in qdev-properties.c or so already?
> 
> cutils.c has:
> 
> int64_t strtosz_suffix(const char *nptr, char **end, const char
> default_suffix) {
>     return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
> }
> 
> $ git grep -w strtosz_suffix
> [...]
> qapi/opts-visitor.c:    val = strtosz_suffix(opt->str ? opt->str : "",
> &endptr, qemu-img.c:        sval = strtosz_suffix(argv[optind++], &end,
> STRTOSZ_DEFSUFFIX_B); qemu-img.c:            sval = strtosz_suffix(optarg,
> &end, STRTOSZ_DEFSUFFIX_B);
> 
> The opts-visitor.c match, in turn, is inside opts_type_size(), that's the
> ->type_size method of OptsVisitor. There are many 'size' elements inside
> qapi-schema.json.
> 
> I don't see any code using visit_type_size() directly, but I see two users
> of type 'size' on qapi-schema.json: NetdevTapOptions and NetdevDumpOptions.
> 
> I didn't know that we already had a visitor method using the suffixed-int
> parsing code. Should we change the visit_type_size() code to be to use use
> the new generic ->type_suffixed_int method and kill ->type_size?

If there isn't strong opposition to do it in incremental way,
I'd prefer for these patches go in first.

And then later fix users of visit_type_size() to use type_suffixed_int() or
maybe have a new type_suffixed_uint() so that size could be represented as
uint64_t instead of int64_t as it's now. That would require to
rewrite strtosz_* and its callers a bit.
Michael Roth - Dec. 10, 2012, 5:21 p.m.
On Mon, Dec 10, 2012 at 05:13:45PM +0100, Igor Mammedov wrote:
> On Fri, 7 Dec 2012 18:09:06 -0200
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Fri, Dec 07, 2012 at 08:00:09PM +0100, Andreas Färber wrote:
> > > Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > ---
> > > >   v2:
> > > >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> > > >      in x86_cpuid_set_tsc_freq()
> > > > ---
> > > >  target-i386/cpu.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > > > index c6c2ca0..b7f0aba 100644
> > > > --- a/target-i386/cpu.c
> > > > +++ b/target-i386/cpu.c
> > > > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj,
> > > > Visitor *v, void *opaque, const int64_t max = INT64_MAX;
> > > >      int64_t value;
> > > >  
> > > > -    visit_type_int(v, &value, name, errp);
> > > > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> > > >      if (error_is_set(errp)) {
> > > >          return;
> > > >      }
> > > 
> > > This trivial usage is fine obviously. But since this series set out to
> > > make things more generic I am missing at least one use case for 1024.
> > > Does nothing like that exist in qdev-properties.c or so already?
> > 
> > cutils.c has:
> > 
> > int64_t strtosz_suffix(const char *nptr, char **end, const char
> > default_suffix) {
> >     return strtosz_suffix_unit(nptr, end, default_suffix, 1024);
> > }
> > 
> > $ git grep -w strtosz_suffix
> > [...]
> > qapi/opts-visitor.c:    val = strtosz_suffix(opt->str ? opt->str : "",
> > &endptr, qemu-img.c:        sval = strtosz_suffix(argv[optind++], &end,
> > STRTOSZ_DEFSUFFIX_B); qemu-img.c:            sval = strtosz_suffix(optarg,
> > &end, STRTOSZ_DEFSUFFIX_B);
> > 
> > The opts-visitor.c match, in turn, is inside opts_type_size(), that's the
> > ->type_size method of OptsVisitor. There are many 'size' elements inside
> > qapi-schema.json.
> > 
> > I don't see any code using visit_type_size() directly, but I see two users
> > of type 'size' on qapi-schema.json: NetdevTapOptions and NetdevDumpOptions.
> > 
> > I didn't know that we already had a visitor method using the suffixed-int
> > parsing code. Should we change the visit_type_size() code to be to use use
> > the new generic ->type_suffixed_int method and kill ->type_size?
> 
> If there isn't strong opposition to do it in incremental way,
> I'd prefer for these patches go in first.
> 
> And then later fix users of visit_type_size() to use type_suffixed_int() or
> maybe have a new type_suffixed_uint() so that size could be represented as
> uint64_t instead of int64_t as it's now. That would require to
> rewrite strtosz_* and its callers a bit.

I think that seems reasonable. Between the 2 that should give us a nice
way to support [KMGx] suffixes for options in a backward-compatible way.

Shame that users don't have a well-established way to specify base 2 vs.
10, but so long as we document our choice for each option that should
cover most cases well enough.
Igor Mammedov - Dec. 10, 2012, 8:47 p.m.
On Fri, 07 Dec 2012 20:00:09 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 06.12.2012 22:12, schrieb Igor Mammedov:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >   v2:
> >    - replace visit_type_freq() with visit_type_unit_suffixed_int()
> >      in x86_cpuid_set_tsc_freq()
> > ---
> >  target-i386/cpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index c6c2ca0..b7f0aba 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1195,7 +1195,7 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
> >      const int64_t max = INT64_MAX;
> >      int64_t value;
> >  
> > -    visit_type_int(v, &value, name, errp);
> > +    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
> >      if (error_is_set(errp)) {
> >          return;
> >      }
> 
> This trivial usage is fine obviously. But since this series set out to
> make things more generic I am missing at least one use case for 1024.
> Does nothing like that exist in qdev-properties.c or so already?
It would be nice to have qdev property for this, perhaps after cpu properties
series we could introduce it and simplify target-i386/cpu.c code a bit.

> 
> Andreas
> 
> -- 
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
>

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index c6c2ca0..b7f0aba 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1195,7 +1195,7 @@  static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque,
     const int64_t max = INT64_MAX;
     int64_t value;
 
-    visit_type_int(v, &value, name, errp);
+    visit_type_unit_suffixed_int(v, &value, name, 1000, errp);
     if (error_is_set(errp)) {
         return;
     }