Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

2012-12-27 Thread Eduardo Habkost 
On Thu, Dec 27, 2012 at 03:59:23PM +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 imamm...@redhat.com

Reviewed-by: Eduardo Habkost ehabk...@redhat.com


 ---
   v2:
- add missing 'return -1' on exit if error is not NULL,
Spotted-By: Eduardo Habkost ehabk...@redhat.com
   v3:
- set error if cpu_model is empty
 ---
  target-i386/cpu.c |   18 +-
  1 files changed, 9 insertions(+), 9 deletions(-)
 
 diff --git a/target-i386/cpu.c b/target-i386/cpu.c
 index 3b9bbfe..97cce89 100644
 --- a/target-i386/cpu.c
 +++ b/target-i386/cpu.c
 @@ -1550,13 +1550,15 @@ int cpu_x86_register(X86CPU *cpu, const char 
 *cpu_model)
  
  model_pieces = g_strsplit(cpu_model, ,, 2);
  if (!model_pieces[0]) {
 -goto error;
 +error_setg(error, Invalid/empty CPU model name);
 +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 +1568,20 @@ 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;
 +return -1;
  }
 -
 -g_strfreev(model_pieces);
  return 0;
 -error:
 -g_strfreev(model_pieces);
 -return -1;
  }
  
  #if !defined(CONFIG_USER_ONLY)
 -- 
 1.7.1
 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

2012-12-18 Thread Eduardo Habkost 
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 imamm...@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
 
 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 07/20] target-i386: cpu_x86_register() consolidate freeing resources

2012-12-18 Thread Igor Mammedov 
On Tue, 18 Dec 2012 13:28:29 -0200
Eduardo Habkost ehabk...@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 imamm...@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