Message ID | 1355760092-18755-16-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
On Mon, Dec 17, 2012 at 05:01:27PM +0100, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > target-i386/cpu.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 0cb0931..714ae79 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, > if (numvalue < 0x80000000) { > numvalue += 0x80000000; > } > - x86_cpu_def->xlevel = numvalue; > + val = g_strdup_printf("%u", numvalue); Why not just eliminate the string->integer conversion entirely, and move it inside x86_cpuid_set_xlevel()? > + qdict_put(*props, featurestr, qstring_from_str(val)); If you made cpu_x86_parse_featurestr() set the properties directly on the object instead of creating an intermediate directionary, it could simply call object_property_set_int() (if for some reason it's impossible to eliminate the string->int conversion above). > + g_free(val); > } else if (!strcmp(featurestr, "vendor")) { > qdict_put(*props, featurestr, qstring_from_str(val)); > x86_cpu_def->vendor_override = true; > -- > 1.7.1 > >
On Wed, 19 Dec 2012 15:58:40 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Mon, Dec 17, 2012 at 05:01:27PM +0100, Igor Mammedov wrote: > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > target-i386/cpu.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 0cb0931..714ae79 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, > > if (numvalue < 0x80000000) { > > numvalue += 0x80000000; > > } > > - x86_cpu_def->xlevel = numvalue; > > + val = g_strdup_printf("%u", numvalue); > > Why not just eliminate the string->integer conversion entirely, and move > it inside x86_cpuid_set_xlevel()? Silent fixup of an incorrect input value doesn't look like a candidate for making into property setter, more correct way is bailing out. But for compatibility sake we can't just remove it without giving userbase a warning. Perhaps candidate for next release notes. And when it's removed, this branch could be collapsed in catch-all(feat=val) branch + adding input value check to xlevel property setter. > > > + qdict_put(*props, featurestr, qstring_from_str(val)); > > If you made cpu_x86_parse_featurestr() set the properties directly on > the object instead of creating an intermediate directionary, it could > simply call object_property_set_int() (if for some reason it's > impossible to eliminate the string->int conversion above). I've explained in answer to [10/20] why. Besides normalizing input first to uniform set and then applying it to object using only one approach whether it via cpu_x86_set_props() or global properties seems to me more straightforward and reduces chance of error. and might allow completely get rid of cpu_x86_parse_featurestr() call in cpu_init() call chain. cpu_x86_parse_featurestr() should/might be called only once to create a set of properties that are applied later to each created CPU. > > > + g_free(val); > > } else if (!strcmp(featurestr, "vendor")) { > > qdict_put(*props, featurestr, qstring_from_str(val)); > > x86_cpu_def->vendor_override = true; > > -- > > 1.7.1 > > > > > > -- > Eduardo
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 0cb0931..714ae79 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1326,7 +1326,9 @@ static int cpu_x86_parse_featurestr(x86_def_t *x86_cpu_def, char *features, if (numvalue < 0x80000000) { numvalue += 0x80000000; } - x86_cpu_def->xlevel = numvalue; + val = g_strdup_printf("%u", numvalue); + qdict_put(*props, featurestr, qstring_from_str(val)); + g_free(val); } else if (!strcmp(featurestr, "vendor")) { qdict_put(*props, featurestr, qstring_from_str(val)); x86_cpu_def->vendor_override = true;
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- target-i386/cpu.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)