diff mbox

[02/22] target-i386: cpu_x86_register(): report error from property setter

Message ID 1347051311-16122-3-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Sept. 7, 2012, 8:54 p.m. UTC 
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 target-i386/cpu.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Eduardo Habkost Sept. 13, 2012, 2:40 p.m. UTC | #1 
On Fri, Sep 07, 2012 at 10:54:51PM +0200, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  target-i386/cpu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> index a89bdc4..3f80069 100644
> --- a/target-i386/cpu.c
> +++ b/target-i386/cpu.c
> @@ -1415,6 +1415,7 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
>  
>  out:
>      if (error_is_set(&error)) {

Isn't "if (error_is_set(&error)) better written as "if (error)"?

There are lots of places where "error_is_set(&error)" is used, but I saw
other QEMU code using "if (error)" before, so I don't know what's the
recommended style.

Anyway, the point of this patch is to add the error message, not
cleaning up the existing code. So:

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

> +        fprintf(stderr, "%s\n", error_get_pretty(error));
>          error_free(error);
>          return -1;
>      }
> -- 
> 1.7.11.4
> 
>
Igor Mammedov Sept. 19, 2012, 3:01 p.m. UTC | #2 
On Thu, 13 Sep 2012 11:40:08 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Fri, Sep 07, 2012 at 10:54:51PM +0200, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  target-i386/cpu.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index a89bdc4..3f80069 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -1415,6 +1415,7 @@ int cpu_x86_register(X86CPU *cpu, const char
> > *cpu_model) 
> >  out:
> >      if (error_is_set(&error)) {
> 
> Isn't "if (error_is_set(&error)) better written as "if (error)"?
> 
> There are lots of places where "error_is_set(&error)" is used, but I saw
> other QEMU code using "if (error)" before, so I don't know what's the
> recommended style.
If there aren't any objections, I'll change it to suggested "if (error)"
style, for the next respin.

> 
> Anyway, the point of this patch is to add the error message, not
> cleaning up the existing code. So:
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> > +        fprintf(stderr, "%s\n", error_get_pretty(error));
> >          error_free(error);
> >          return -1;
> >      }
> > -- 
> > 1.7.11.4
> > 
> > 
>
diff mbox

Patch

diff --git a/target-i386/cpu.c b/target-i386/cpu.c
index a89bdc4..3f80069 100644
--- a/target-i386/cpu.c
+++ b/target-i386/cpu.c
@@ -1415,6 +1415,7 @@  int cpu_x86_register(X86CPU *cpu, const char *cpu_model)
 
 out:
     if (error_is_set(&error)) {
+        fprintf(stderr, "%s\n", error_get_pretty(error));
         error_free(error);
         return -1;
     }