| Message ID | 1349192235-31895-2-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 02.10.2012 17:36, schrieb Igor Mammedov: > it will allow to use property setters there later. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Don Slutz <Don@CloudSwitch.com> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > -- > v2: > - style change, add braces (reqested by Blue Swirl) > - removed unused error_is_set(errp) in properties set loop > --- > target-i386/cpu.c | 15 ++++++++++++--- > 1 files changed, 12 insertions(+), 3 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index bb1e44e..e1ffa40 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -1097,7 +1097,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque, > cpu->env.tsc_khz = value / 1000; > } > > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, > + const char *cpu_model, Error **errp) > { > unsigned int i; > x86_def_t *def; > @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); > goto error; > } > + > featurestr = strtok(NULL, ","); > } > x86_cpu_def->features |= plus_features; Disconnected whitespace change. > @@ -1282,6 +1284,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) > > error: > g_free(s); > + if (!error_is_set(errp)) { > + error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); Note to self: error_set() checks for errp being NULL, so the use of !error_is_set() logic seems fine. QERR_ alarm - acceptable use? Or better use error_setg() or something? Otherwise looks okay to me, I could drop the whitespace hunk myself. Andreas > + } > return -1; > } > > @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > memset(def, 0, sizeof(*def)); > > - if (cpu_x86_find_by_name(def, cpu_model) < 0) > - return -1; > + if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) { > + goto out; > + } > + > if (def->vendor1) { > env->cpuid_vendor1 = def->vendor1; > env->cpuid_vendor2 = def->vendor2; > @@ -1419,6 +1426,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > env->cpuid_svm_features &= TCG_SVM_FEATURES; > } > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error); > + > +out: > if (error_is_set(&error)) { > error_free(error); > return -1; >
On Wed, 10 Oct 2012 16:05:10 +0200 Andreas Färber <afaerber@suse.de> wrote: > Am 02.10.2012 17:36, schrieb Igor Mammedov: > > it will allow to use property setters there later. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Reviewed-by: Don Slutz <Don@CloudSwitch.com> > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > -- > > v2: > > - style change, add braces (reqested by Blue Swirl) > > - removed unused error_is_set(errp) in properties set loop > > --- > > target-i386/cpu.c | 15 ++++++++++++--- > > 1 files changed, 12 insertions(+), 3 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index bb1e44e..e1ffa40 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -1097,7 +1097,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, > > Visitor *v, void *opaque, cpu->env.tsc_khz = value / 1000; > > } > > > > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char > > *cpu_model) +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t > > *x86_cpu_def, > > + const char *cpu_model, Error **errp) > > { > > unsigned int i; > > x86_def_t *def; > > > @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t > > *x86_cpu_def, const char *cpu_model) fprintf(stderr, "feature string `%s' > > not in format (+feature|-feature|feature=xyz)\n", featurestr); goto error; > > } > > + > > featurestr = strtok(NULL, ","); > > } > > x86_cpu_def->features |= plus_features; > > Disconnected whitespace change. > > > @@ -1282,6 +1284,9 @@ static int cpu_x86_find_by_name(x86_def_t > > *x86_cpu_def, const char *cpu_model) > > error: > > g_free(s); > > + if (!error_is_set(errp)) { > > + error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); > > Note to self: error_set() checks for errp being NULL, so the use of > !error_is_set() logic seems fine. > > QERR_ alarm - acceptable use? Or better use error_setg() or something? > > Otherwise looks okay to me, I could drop the whitespace hunk myself. I'll fix it for the next re-spin of this series and do whatever is decided with QERR_... Thanks! > > Andreas > > > + } > > return -1; > > } > > > > @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char > > *cpu_model) > > memset(def, 0, sizeof(*def)); > > > > - if (cpu_x86_find_by_name(def, cpu_model) < 0) > > - return -1; > > + if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) { > > + goto out; > > + } > > + > > if (def->vendor1) { > > env->cpuid_vendor1 = def->vendor1; > > env->cpuid_vendor2 = def->vendor2; > > @@ -1419,6 +1426,8 @@ int cpu_x86_register(X86CPU *cpu, const char > > *cpu_model) env->cpuid_svm_features &= TCG_SVM_FEATURES; > > } > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", > > &error); + > > +out: > > if (error_is_set(&error)) { > > error_free(error); > > return -1; > > > >
On Wed, 10 Oct 2012 16:38:10 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Wed, 10 Oct 2012 16:05:10 +0200 > Andreas Färber <afaerber@suse.de> wrote: > > > Am 02.10.2012 17:36, schrieb Igor Mammedov: > > > it will allow to use property setters there later. > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > > Reviewed-by: Don Slutz <Don@CloudSwitch.com> > > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > > > -- > > > v2: > > > - style change, add braces (reqested by Blue Swirl) > > > - removed unused error_is_set(errp) in properties set loop > > > --- > > > target-i386/cpu.c | 15 ++++++++++++--- > > > 1 files changed, 12 insertions(+), 3 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index bb1e44e..e1ffa40 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -1097,7 +1097,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, > > > Visitor *v, void *opaque, cpu->env.tsc_khz = value / 1000; > > > } > > > > > > -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char > > > *cpu_model) +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t > > > *x86_cpu_def, > > > + const char *cpu_model, Error **errp) > > > { > > > unsigned int i; > > > x86_def_t *def; > > > > > @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t > > > *x86_cpu_def, const char *cpu_model) fprintf(stderr, "feature string `%s' > > > not in format (+feature|-feature|feature=xyz)\n", featurestr); goto error; > > > } > > > + > > > featurestr = strtok(NULL, ","); > > > } > > > x86_cpu_def->features |= plus_features; > > > > Disconnected whitespace change. > > > > > @@ -1282,6 +1284,9 @@ static int cpu_x86_find_by_name(x86_def_t > > > *x86_cpu_def, const char *cpu_model) > > > error: > > > g_free(s); > > > + if (!error_is_set(errp)) { > > > + error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); > > > > Note to self: error_set() checks for errp being NULL, so the use of > > !error_is_set() logic seems fine. > > > > QERR_ alarm - acceptable use? Or better use error_setg() or something? > > > > Otherwise looks okay to me, I could drop the whitespace hunk myself. > I'll fix it for the next re-spin of this series and do whatever is decided > with QERR_... It's simpler than you can imagine, you can just do: error_setg(errp, "invalid parameter combination: %s", params); > > Thanks! > > > > > Andreas > > > > > + } > > > return -1; > > > } > > > > > > @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char > > > *cpu_model) > > > memset(def, 0, sizeof(*def)); > > > > > > - if (cpu_x86_find_by_name(def, cpu_model) < 0) > > > - return -1; > > > + if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) { > > > + goto out; > > > + } > > > + > > > if (def->vendor1) { > > > env->cpuid_vendor1 = def->vendor1; > > > env->cpuid_vendor2 = def->vendor2; > > > @@ -1419,6 +1426,8 @@ int cpu_x86_register(X86CPU *cpu, const char > > > *cpu_model) env->cpuid_svm_features &= TCG_SVM_FEATURES; > > > } > > > object_property_set_str(OBJECT(cpu), def->model_id, "model-id", > > > &error); + > > > +out: > > > if (error_is_set(&error)) { > > > error_free(error); > > > return -1; > > > > > > > >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index bb1e44e..e1ffa40 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -1097,7 +1097,8 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, void *opaque, cpu->env.tsc_khz = value / 1000; } -static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) +static int cpu_x86_find_by_name(X86CPU *cpu, x86_def_t *x86_cpu_def, + const char *cpu_model, Error **errp) { unsigned int i; x86_def_t *def; @@ -1254,6 +1255,7 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) fprintf(stderr, "feature string `%s' not in format (+feature|-feature|feature=xyz)\n", featurestr); goto error; } + featurestr = strtok(NULL, ","); } x86_cpu_def->features |= plus_features; @@ -1282,6 +1284,9 @@ static int cpu_x86_find_by_name(x86_def_t *x86_cpu_def, const char *cpu_model) error: g_free(s); + if (!error_is_set(errp)) { + error_set(errp, QERR_INVALID_PARAMETER_COMBINATION); + } return -1; } @@ -1368,8 +1373,10 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) memset(def, 0, sizeof(*def)); - if (cpu_x86_find_by_name(def, cpu_model) < 0) - return -1; + if (cpu_x86_find_by_name(cpu, def, cpu_model, &error) < 0) { + goto out; + } + if (def->vendor1) { env->cpuid_vendor1 = def->vendor1; env->cpuid_vendor2 = def->vendor2; @@ -1419,6 +1426,8 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) env->cpuid_svm_features &= TCG_SVM_FEATURES; } object_property_set_str(OBJECT(cpu), def->model_id, "model-id", &error); + +out: if (error_is_set(&error)) { error_free(error); return -1;