| Message ID | 1393901749-5944-2-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
On Tue, 4 Mar 2014 03:55:44 +0100 Andreas Färber <afaerber@suse.de> wrote: > Adapt the X86CPU implementation to suit the generic hook. > This involves a cleanup of error handling to cope with NULL errp. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > include/qom/cpu.h | 3 +++ > target-i386/cpu.c | 36 +++++++++++++++++++++--------------- > 2 files changed, 24 insertions(+), 15 deletions(-) > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 64ebfa5..43d253a 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -67,6 +67,8 @@ struct TranslationBlock; > * CPUClass: > * @class_by_name: Callback to map -cpu command line model name to an > * instantiatable CPU type. > + * @parse_features: Callback to parse command line arguments. > + * The argument may be modified by the callback. Could you specify which argument is expected to be modified? > * @reset: Callback to reset the #CPUState to its initial state. > * @reset_dump_flags: #CPUDumpFlags to use for reset logging. > * @has_work: Callback for checking if there is work to do. > @@ -96,6 +98,7 @@ typedef struct CPUClass { > /*< public >*/ > > ObjectClass *(*class_by_name)(const char *cpu_model); > + void (*parse_features)(CPUState *cpu, char *str, Error **errp); > > void (*reset)(CPUState *cpu); > int reset_dump_flags; > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index cbc8cd6..653840a 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1626,8 +1626,10 @@ static inline void feat2prop(char *s) > > /* Parse "+feature,-feature,feature=foo" CPU feature string > */ > -static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) > +static void x86_cpu_parse_featurestr(CPUState *cs, char *features, > + Error **errp) > { > + X86CPU *cpu = X86_CPU(cs); > char *featurestr; /* Single 'key=value" string being parsed */ > /* Features to be added */ > FeatureWordArray plus_features = { 0 }; > @@ -1635,6 +1637,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) > FeatureWordArray minus_features = { 0 }; > uint32_t numvalue; > CPUX86State *env = &cpu->env; > + Error *local_err = NULL; > > featurestr = features ? strtok(features, ",") : NULL; > > @@ -1653,16 +1656,16 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) > > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > - error_setg(errp, "bad numerical value %s", val); > + error_setg(&local_err, "bad numerical value %s", val); > goto out; > } > if (numvalue < 0x80000000) { > - fprintf(stderr, "xlevel value shall always be >= 0x80000000" > - ", fixup will be removed in future versions\n"); > + error_report("xlevel value shall always be >= 0x80000000" > + ", fixup will be removed in future versions"); > numvalue += 0x80000000; > } > snprintf(num, sizeof(num), "%" PRIu32, numvalue); > - object_property_parse(OBJECT(cpu), num, featurestr, errp); > + object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > } else if (!strcmp(featurestr, "tsc-freq")) { > int64_t tsc_freq; > char *err; > @@ -1671,36 +1674,38 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) > tsc_freq = strtosz_suffix_unit(val, &err, > STRTOSZ_DEFSUFFIX_B, 1000); > if (tsc_freq < 0 || *err) { > - error_setg(errp, "bad numerical value %s", val); > + error_setg(&local_err, "bad numerical value %s", val); > goto out; > } > snprintf(num, sizeof(num), "%" PRId64, tsc_freq); > - object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp); > + object_property_parse(OBJECT(cpu), num, "tsc-frequency", > + &local_err); > } else if (!strcmp(featurestr, "hv-spinlocks")) { > char *err; > const int min = 0xFFF; > char num[32]; > numvalue = strtoul(val, &err, 0); > if (!*val || *err) { > - error_setg(errp, "bad numerical value %s", val); > + error_setg(&local_err, "bad numerical value %s", val); > goto out; > } > if (numvalue < min) { > - fprintf(stderr, "hv-spinlocks value shall always be >= 0x%x" > - ", fixup will be removed in future versions\n", > + error_report("hv-spinlocks value shall always be >= 0x%x" > + ", fixup will be removed in future versions", > min); > numvalue = min; > } > snprintf(num, sizeof(num), "%" PRId32, numvalue); > - object_property_parse(OBJECT(cpu), num, featurestr, errp); > + object_property_parse(OBJECT(cpu), num, featurestr, &local_err); > } else { > - object_property_parse(OBJECT(cpu), val, featurestr, errp); > + object_property_parse(OBJECT(cpu), val, featurestr, &local_err); > } > } else { > feat2prop(featurestr); > - object_property_parse(OBJECT(cpu), "on", featurestr, errp); > + object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); > } > - if (error_is_set(errp)) { > + if (local_err) { > + error_propagate(errp, local_err); > goto out; > } > featurestr = strtok(NULL, ","); > @@ -1926,7 +1931,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, > goto out; > } > > - cpu_x86_parse_featurestr(cpu, features, &error); > + x86_cpu_parse_featurestr(CPU(cpu), features, &error); > if (error) { > goto out; > } > @@ -2748,6 +2753,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) > cc->reset = x86_cpu_reset; > cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; > > + cc->parse_features = x86_cpu_parse_featurestr; > cc->has_work = x86_cpu_has_work; > cc->do_interrupt = x86_cpu_do_interrupt; > cc->dump_state = x86_cpu_dump_state;
Am 05.03.2014 16:04, schrieb Igor Mammedov: > On Tue, 4 Mar 2014 03:55:44 +0100 > Andreas Färber <afaerber@suse.de> wrote: > >> Adapt the X86CPU implementation to suit the generic hook. >> This involves a cleanup of error handling to cope with NULL errp. >> >> Signed-off-by: Andreas Färber <afaerber@suse.de> >> --- >> include/qom/cpu.h | 3 +++ >> target-i386/cpu.c | 36 +++++++++++++++++++++--------------- >> 2 files changed, 24 insertions(+), 15 deletions(-) >> >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h >> index 64ebfa5..43d253a 100644 >> --- a/include/qom/cpu.h >> +++ b/include/qom/cpu.h >> @@ -67,6 +67,8 @@ struct TranslationBlock; >> * CPUClass: >> * @class_by_name: Callback to map -cpu command line model name to an >> * instantiatable CPU type. >> + * @parse_features: Callback to parse command line arguments. >> + * The argument may be modified by the callback. > Could you specify which argument is expected to be modified? Like so? "The arguments (%str) may be modified by the callback." Alternatively I could drop that line, given that it's not const char *. Or add a typedef for the callback and document it there using @str syntax. Thanks, Andreas > >> * @reset: Callback to reset the #CPUState to its initial state. >> * @reset_dump_flags: #CPUDumpFlags to use for reset logging. >> * @has_work: Callback for checking if there is work to do. >> @@ -96,6 +98,7 @@ typedef struct CPUClass { >> /*< public >*/ >> >> ObjectClass *(*class_by_name)(const char *cpu_model); >> + void (*parse_features)(CPUState *cpu, char *str, Error **errp); >> >> void (*reset)(CPUState *cpu); >> int reset_dump_flags; [snip]
On Wed, 05 Mar 2014 17:06:15 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 05.03.2014 16:04, schrieb Igor Mammedov: > > On Tue, 4 Mar 2014 03:55:44 +0100 > > Andreas Färber <afaerber@suse.de> wrote: > > > >> Adapt the X86CPU implementation to suit the generic hook. > >> This involves a cleanup of error handling to cope with NULL errp. > >> > >> Signed-off-by: Andreas Färber <afaerber@suse.de> > >> --- > >> include/qom/cpu.h | 3 +++ > >> target-i386/cpu.c | 36 +++++++++++++++++++++--------------- > >> 2 files changed, 24 insertions(+), 15 deletions(-) > >> > >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h > >> index 64ebfa5..43d253a 100644 > >> --- a/include/qom/cpu.h > >> +++ b/include/qom/cpu.h > >> @@ -67,6 +67,8 @@ struct TranslationBlock; > >> * CPUClass: > >> * @class_by_name: Callback to map -cpu command line model name to an > >> * instantiatable CPU type. > >> + * @parse_features: Callback to parse command line arguments. > >> + * The argument may be modified by the callback. > > Could you specify which argument is expected to be modified? > > Like so? "The arguments (%str) may be modified by the callback." > > Alternatively I could drop that line, given that it's not const char *. > Or add a typedef for the callback and document it there using @str syntax. I'd prefer to drop it. BTW: why is 'str' modified by callback? > Thanks, > Andreas > > > > >> * @reset: Callback to reset the #CPUState to its initial state. > >> * @reset_dump_flags: #CPUDumpFlags to use for reset logging. > >> * @has_work: Callback for checking if there is work to do. > >> @@ -96,6 +98,7 @@ typedef struct CPUClass { > >> /*< public >*/ > >> > >> ObjectClass *(*class_by_name)(const char *cpu_model); > >> + void (*parse_features)(CPUState *cpu, char *str, Error **errp); > >> > >> void (*reset)(CPUState *cpu); > >> int reset_dump_flags; > [snip] >
On Wed, Mar 05, 2014 at 05:57:10PM +0100, Igor Mammedov wrote: > On Wed, 05 Mar 2014 17:06:15 +0100 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 05.03.2014 16:04, schrieb Igor Mammedov: > > > On Tue, 4 Mar 2014 03:55:44 +0100 > > > Andreas Färber <afaerber@suse.de> wrote: > > > > > >> Adapt the X86CPU implementation to suit the generic hook. > > >> This involves a cleanup of error handling to cope with NULL errp. > > >> > > >> Signed-off-by: Andreas Färber <afaerber@suse.de> > > >> --- > > >> include/qom/cpu.h | 3 +++ > > >> target-i386/cpu.c | 36 +++++++++++++++++++++--------------- > > >> 2 files changed, 24 insertions(+), 15 deletions(-) > > >> > > >> diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > >> index 64ebfa5..43d253a 100644 > > >> --- a/include/qom/cpu.h > > >> +++ b/include/qom/cpu.h > > >> @@ -67,6 +67,8 @@ struct TranslationBlock; > > >> * CPUClass: > > >> * @class_by_name: Callback to map -cpu command line model name to an > > >> * instantiatable CPU type. > > >> + * @parse_features: Callback to parse command line arguments. > > >> + * The argument may be modified by the callback. > > > Could you specify which argument is expected to be modified? > > > > Like so? "The arguments (%str) may be modified by the callback." > > > > Alternatively I could drop that line, given that it's not const char *. > > Or add a typedef for the callback and document it there using @str syntax. > I'd prefer to drop it. > > BTW: why is 'str' modified by callback? Allowing it to be modified allows (for example) strtok() to be used (like we do on the i386 code today). So I don't see a reason to forbid it.
Am 04.03.2014 03:55, schrieb Andreas Färber: > Adapt the X86CPU implementation to suit the generic hook. > This involves a cleanup of error handling to cope with NULL errp. > > Signed-off-by: Andreas Färber <afaerber@suse.de> > --- > include/qom/cpu.h | 3 +++ > target-i386/cpu.c | 36 +++++++++++++++++++++--------------- > 2 files changed, 24 insertions(+), 15 deletions(-) X86CPU subclasses require the following trivial rebase: diff --cc target-i386/cpu.c index 3c3a987,653840a..0000000 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@@ -1941,7 -1923,15 +1946,7 @@@ X86CPU *cpu_x86_create(const char *cpu_ object_unref(OBJECT(cpu)); #endif - cpu_x86_parse_featurestr(cpu, features, &error); - /* Emulate per-model subclasses for global properties */ - typename = g_strdup_printf("%s-" TYPE_X86_CPU, name); - qdev_prop_set_globals_for_type(DEVICE(cpu), typename, &error); - g_free(typename); - if (error) { - goto out; - } - + x86_cpu_parse_featurestr(CPU(cpu), features, &error); if (error) { goto out; } @@@ -2790,7 -2753,7 +2795,8 @@@ static void x86_cpu_common_class_init(O cc->reset = x86_cpu_reset; cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; + cc->class_by_name = x86_cpu_class_by_name; + cc->parse_features = x86_cpu_parse_featurestr; cc->has_work = x86_cpu_has_work; cc->do_interrupt = x86_cpu_do_interrupt; cc->dump_state = x86_cpu_dump_state; Andreas
On Sun, 09 Mar 2014 16:45:20 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 04.03.2014 03:55, schrieb Andreas Färber: > > Adapt the X86CPU implementation to suit the generic hook. > > This involves a cleanup of error handling to cope with NULL errp. > > > > Signed-off-by: Andreas Färber <afaerber@suse.de> > > --- > > include/qom/cpu.h | 3 +++ > > target-i386/cpu.c | 36 +++++++++++++++++++++--------------- > > 2 files changed, 24 insertions(+), 15 deletions(-) > > X86CPU subclasses require the following trivial rebase: > > diff --cc target-i386/cpu.c > index 3c3a987,653840a..0000000 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@@ -1941,7 -1923,15 +1946,7 @@@ X86CPU *cpu_x86_create(const char *cpu_ > object_unref(OBJECT(cpu)); > #endif > > - cpu_x86_parse_featurestr(cpu, features, &error); > - /* Emulate per-model subclasses for global properties */ > - typename = g_strdup_printf("%s-" TYPE_X86_CPU, name); > - qdev_prop_set_globals_for_type(DEVICE(cpu), typename, &error); > - g_free(typename); > - if (error) { > - goto out; > - } > - > + x86_cpu_parse_featurestr(CPU(cpu), features, &error); > if (error) { > goto out; > } > @@@ -2790,7 -2753,7 +2795,8 @@@ static void x86_cpu_common_class_init(O > cc->reset = x86_cpu_reset; > cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; > > + cc->class_by_name = x86_cpu_class_by_name; > + cc->parse_features = x86_cpu_parse_featurestr; > cc->has_work = x86_cpu_has_work; > cc->do_interrupt = x86_cpu_do_interrupt; > cc->dump_state = x86_cpu_dump_state; > > Andreas > looks good, Reviewed-by: Igor Mammedov <imammedo@redhat.com>
diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 64ebfa5..43d253a 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -67,6 +67,8 @@ struct TranslationBlock; * CPUClass: * @class_by_name: Callback to map -cpu command line model name to an * instantiatable CPU type. + * @parse_features: Callback to parse command line arguments. + * The argument may be modified by the callback. * @reset: Callback to reset the #CPUState to its initial state. * @reset_dump_flags: #CPUDumpFlags to use for reset logging. * @has_work: Callback for checking if there is work to do. @@ -96,6 +98,7 @@ typedef struct CPUClass { /*< public >*/ ObjectClass *(*class_by_name)(const char *cpu_model); + void (*parse_features)(CPUState *cpu, char *str, Error **errp); void (*reset)(CPUState *cpu); int reset_dump_flags; diff --git a/target-i386/cpu.c b/target-i386/cpu.c index cbc8cd6..653840a 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1626,8 +1626,10 @@ static inline void feat2prop(char *s) /* Parse "+feature,-feature,feature=foo" CPU feature string */ -static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) +static void x86_cpu_parse_featurestr(CPUState *cs, char *features, + Error **errp) { + X86CPU *cpu = X86_CPU(cs); char *featurestr; /* Single 'key=value" string being parsed */ /* Features to be added */ FeatureWordArray plus_features = { 0 }; @@ -1635,6 +1637,7 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) FeatureWordArray minus_features = { 0 }; uint32_t numvalue; CPUX86State *env = &cpu->env; + Error *local_err = NULL; featurestr = features ? strtok(features, ",") : NULL; @@ -1653,16 +1656,16 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) numvalue = strtoul(val, &err, 0); if (!*val || *err) { - error_setg(errp, "bad numerical value %s", val); + error_setg(&local_err, "bad numerical value %s", val); goto out; } if (numvalue < 0x80000000) { - fprintf(stderr, "xlevel value shall always be >= 0x80000000" - ", fixup will be removed in future versions\n"); + error_report("xlevel value shall always be >= 0x80000000" + ", fixup will be removed in future versions"); numvalue += 0x80000000; } snprintf(num, sizeof(num), "%" PRIu32, numvalue); - object_property_parse(OBJECT(cpu), num, featurestr, errp); + object_property_parse(OBJECT(cpu), num, featurestr, &local_err); } else if (!strcmp(featurestr, "tsc-freq")) { int64_t tsc_freq; char *err; @@ -1671,36 +1674,38 @@ static void cpu_x86_parse_featurestr(X86CPU *cpu, char *features, Error **errp) tsc_freq = strtosz_suffix_unit(val, &err, STRTOSZ_DEFSUFFIX_B, 1000); if (tsc_freq < 0 || *err) { - error_setg(errp, "bad numerical value %s", val); + error_setg(&local_err, "bad numerical value %s", val); goto out; } snprintf(num, sizeof(num), "%" PRId64, tsc_freq); - object_property_parse(OBJECT(cpu), num, "tsc-frequency", errp); + object_property_parse(OBJECT(cpu), num, "tsc-frequency", + &local_err); } else if (!strcmp(featurestr, "hv-spinlocks")) { char *err; const int min = 0xFFF; char num[32]; numvalue = strtoul(val, &err, 0); if (!*val || *err) { - error_setg(errp, "bad numerical value %s", val); + error_setg(&local_err, "bad numerical value %s", val); goto out; } if (numvalue < min) { - fprintf(stderr, "hv-spinlocks value shall always be >= 0x%x" - ", fixup will be removed in future versions\n", + error_report("hv-spinlocks value shall always be >= 0x%x" + ", fixup will be removed in future versions", min); numvalue = min; } snprintf(num, sizeof(num), "%" PRId32, numvalue); - object_property_parse(OBJECT(cpu), num, featurestr, errp); + object_property_parse(OBJECT(cpu), num, featurestr, &local_err); } else { - object_property_parse(OBJECT(cpu), val, featurestr, errp); + object_property_parse(OBJECT(cpu), val, featurestr, &local_err); } } else { feat2prop(featurestr); - object_property_parse(OBJECT(cpu), "on", featurestr, errp); + object_property_parse(OBJECT(cpu), "on", featurestr, &local_err); } - if (error_is_set(errp)) { + if (local_err) { + error_propagate(errp, local_err); goto out; } featurestr = strtok(NULL, ","); @@ -1926,7 +1931,7 @@ X86CPU *cpu_x86_create(const char *cpu_model, DeviceState *icc_bridge, goto out; } - cpu_x86_parse_featurestr(cpu, features, &error); + x86_cpu_parse_featurestr(CPU(cpu), features, &error); if (error) { goto out; } @@ -2748,6 +2753,7 @@ static void x86_cpu_common_class_init(ObjectClass *oc, void *data) cc->reset = x86_cpu_reset; cc->reset_dump_flags = CPU_DUMP_FPU | CPU_DUMP_CCOP; + cc->parse_features = x86_cpu_parse_featurestr; cc->has_work = x86_cpu_has_work; cc->do_interrupt = x86_cpu_do_interrupt; cc->dump_state = x86_cpu_dump_state;
Adapt the X86CPU implementation to suit the generic hook. This involves a cleanup of error handling to cope with NULL errp. Signed-off-by: Andreas Färber <afaerber@suse.de> --- include/qom/cpu.h | 3 +++ target-i386/cpu.c | 36 +++++++++++++++++++++--------------- 2 files changed, 24 insertions(+), 15 deletions(-)