diff mbox

[v2,1/6] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

Message ID 1465492263-28472-2-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov June 9, 2016, 5:10 p.m. UTC 
From: Eduardo Habkost <ehabkost@redhat.com>

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
---
v1:
 - fix error handling in of +-feat, Igor Mammedov <imammedo@redhat.com>
 - rebase on top of
    "target-i386: Remove xlevel & hv-spinlocks option fixups"
v2:
 - move error_propagate() out of loop.
       Eduardo Habkost <ehabkost@redhat.com>
---
 target-i386/cpu.c | 74 +++++++++++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 29 deletions(-)

Comments

Eric Blake June 9, 2016, 7:39 p.m. UTC | #1 
On 06/09/2016 11:29 AM, Eduardo Habkost wrote:
> On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> [...]
>> +
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>>      }
> 
> error_propagate() already ignores local_err==NULL so you don't
> need to check it first.

In fact, if the ONLY reason you are doing an 'if (local_err)'
conditional is to decide if an error was set, then you don't care about
the error locally, and could have passed errp instead of &local_err in
the first place.
Eduardo Habkost June 9, 2016, 7:50 p.m. UTC | #2 
On Thu, Jun 09, 2016 at 01:39:34PM -0600, Eric Blake wrote:
> On 06/09/2016 11:29 AM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> > [...]
> >> +
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >>      }
> > 
> > error_propagate() already ignores local_err==NULL so you don't
> > need to check it first.
> 
> In fact, if the ONLY reason you are doing an 'if (local_err)'
> conditional is to decide if an error was set, then you don't care about
> the error locally, and could have passed errp instead of &local_err in
> the first place.

In this case, we stop the parsing loop if an error is detected.
Igor Mammedov June 10, 2016, 7:23 a.m. UTC | #3 
On Thu, 9 Jun 2016 13:39:34 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 06/09/2016 11:29 AM, Eduardo Habkost wrote:
> > On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> > [...]  
> >> +
> >> +    if (local_err) {
> >> +        error_propagate(errp, local_err);
> >>      }  
> > 
> > error_propagate() already ignores local_err==NULL so you don't
> > need to check it first.  
> 
> In fact, if the ONLY reason you are doing an 'if (local_err)'
> conditional is to decide if an error was set, then you don't care about
> the error locally, and could have passed errp instead of &local_err in
> the first place.
errp might be NULL, so we won't do parsing at all.
considering it's more or less generic API hook it's safer
to follow error handling pattern used in properties
i.e. use local_error internally and then propagate error
if caller cares about it.
Igor Mammedov June 10, 2016, 7:24 a.m. UTC | #4 
On Thu, 9 Jun 2016 14:29:00 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Thu, Jun 09, 2016 at 07:10:58PM +0200, Igor Mammedov wrote:
> [...]
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> >      }  
> 
> error_propagate() already ignores local_err==NULL so you don't
> need to check it first.
> 
> I can change this while applying the patch, if you're OK.
sounds good to me.

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

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 0e6d342..947cf18 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1965,43 +1965,59 @@  static void x86_cpu_parse_featurestr(CPUState *cs, char *features,
     char *featurestr; /* Single 'key=value" string being parsed */
     Error *local_err = NULL;
 
-    featurestr = features ? strtok(features, ",") : NULL;
+    if (!features) {
+        return;
+    }
+
+    for (featurestr = strtok(features, ",");
+         featurestr  && !local_err;
+         featurestr = strtok(NULL, ",")) {
+        const char *name;
+        const char *val = NULL;
+        char *eq = NULL;
 
-    while (featurestr) {
-        char *val;
+        /* Compatibility syntax: */
         if (featurestr[0] == '+') {
             add_flagname_to_bitmaps(featurestr + 1, plus_features, &local_err);
+            continue;
         } else if (featurestr[0] == '-') {
             add_flagname_to_bitmaps(featurestr + 1, minus_features, &local_err);
-        } else if ((val = strchr(featurestr, '='))) {
-            *val = 0; val++;
-            feat2prop(featurestr);
-            if (!strcmp(featurestr, "tsc-freq")) {
-                int64_t tsc_freq;
-                char *err;
-                char num[32];
-
-                tsc_freq = qemu_strtosz_suffix_unit(val, &err,
-                                               QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
-                if (tsc_freq < 0 || *err) {
-                    error_setg(errp, "bad numerical value %s", val);
-                    return;
-                }
-                snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
-                object_property_parse(OBJECT(cpu), num, "tsc-frequency",
-                                      &local_err);
-            } else {
-                object_property_parse(OBJECT(cpu), val, featurestr, &local_err);
-            }
+            continue;
+        }
+
+        eq = strchr(featurestr, '=');
+        if (eq) {
+            *eq++ = 0;
+            val = eq;
         } else {
-            feat2prop(featurestr);
-            object_property_parse(OBJECT(cpu), "on", featurestr, &local_err);
+            val = "on";
         }
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
+
+        feat2prop(featurestr);
+        name = featurestr;
+
+        /* Special case: */
+        if (!strcmp(name, "tsc-freq")) {
+            int64_t tsc_freq;
+            char *err;
+            char num[32];
+
+            tsc_freq = qemu_strtosz_suffix_unit(val, &err,
+                                           QEMU_STRTOSZ_DEFSUFFIX_B, 1000);
+            if (tsc_freq < 0 || *err) {
+                error_setg(errp, "bad numerical value %s", val);
+                return;
+            }
+            snprintf(num, sizeof(num), "%" PRId64, tsc_freq);
+            val = num;
+            name = "tsc-frequency";
         }
-        featurestr = strtok(NULL, ",");
+
+        object_property_parse(OBJECT(cpu), val, name, &local_err);
+    }
+
+    if (local_err) {
+        error_propagate(errp, local_err);
     }
 }