Patchwork [07/20] target-i386: cpu_x86_register() consolidate freeing resources

login
register
mail settings
Submitter Igor Mammedov
Date Dec. 17, 2012, 4:01 p.m.
Message ID <1355760092-18755-8-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/206934/
State New
Headers show

Comments

Igor Mammedov - Dec. 17, 2012, 4:01 p.m.
freeing resources in one place would require setting 'error'
to not NULL, so add some more error reporting before jumping to
exit branch.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)
Eduardo Habkost - Dec. 18, 2012, 3:28 p.m.
On Mon, Dec 17, 2012 at 05:01:19PM +0100, Igor Mammedov wrote:
> freeing resources in one place would require setting 'error'
> to not NULL, so add some more error reporting before jumping to
> exit branch.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
[...]
> +out:
> +    g_strfreev(model_pieces);
>      if (error) {
>          fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
> -        goto error;
>      }
> -
> -    g_strfreev(model_pieces);


You are making the function return 0 on errors. Is it on purpose?


>      return 0;
> -error:
> -    g_strfreev(model_pieces);
> -    return -1;
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> -- 
> 1.7.1
> 
>
Igor Mammedov - Dec. 18, 2012, 4:15 p.m.
On Tue, 18 Dec 2012 13:28:29 -0200
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Dec 17, 2012 at 05:01:19PM +0100, Igor Mammedov wrote:
> > freeing resources in one place would require setting 'error'
> > to not NULL, so add some more error reporting before jumping to
> > exit branch.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> [...]
> > +out:
> > +    g_strfreev(model_pieces);
> >      if (error) {
> >          fprintf(stderr, "%s\n", error_get_pretty(error));
> >          error_free(error);
> > -        goto error;
> >      }
> > -
> > -    g_strfreev(model_pieces);
> 
> 
> You are making the function return 0 on errors. Is it on purpose?
Nope, I'm sorry it seems I've lost return -1; in error branch on the way.

> 
> 
> >      return 0;
> > -error:
> > -    g_strfreev(model_pieces);
> > -    return -1;
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -- 
> > 1.7.1
> > 
> > 
>

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index 3b9bbfe..418c899 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1550,13 +1550,14 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
     model_pieces = g_strsplit(cpu_model, ",", 2);
     if (!model_pieces[0]) {
-        goto error;
+        goto out;
     }
     name = model_pieces[0];
     features = model_pieces[1];
 
     if (cpu_x86_find_by_name(def, name) < 0) {
-        goto error;
+        error_setg(&error, "Unable to find CPU definition: %s", name);
+        goto out;
     }
 
     def->kvm_features |= kvm_default_features;
@@ -1566,22 +1567,19 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
                             &def->svm_features, &def->cpuid_7_0_ebx_features);
 
     if (cpu_x86_parse_featurestr(def, features) < 0) {
-        goto error;
+        error_setg(&error, "Invalid cpu_model string format: %s", cpu_model);
+        goto out;
     }
 
     cpudef_2_x86_cpu(cpu, def, &error);
 
+out:
+    g_strfreev(model_pieces);
     if (error) {
         fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
-        goto error;
     }
-
-    g_strfreev(model_pieces);
     return 0;
-error:
-    g_strfreev(model_pieces);
-    return -1;
 }
 
 #if !defined(CONFIG_USER_ONLY)