| Message ID | 1354828325-16568-3-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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
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?
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.
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.
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 >
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; }
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(-)