Patchwork [03/19] Remove double error message in qemu_option_set()

login
register
mail settings
Submitter Mark McLoughlin
Date Sept. 10, 2009, 3:18 p.m.
Message ID <1252595941-15196-4-git-send-email-markmc@redhat.com>
Download mbox | patch
Permalink /patch/33326/
State Superseded
Headers show

Comments

Mark McLoughlin - Sept. 10, 2009, 3:18 p.m.
qemu_opt_set() prints an error message in all failure cases, so
qemu_set_option() doesn't need to print another error.

Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 qemu-config.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
Michael S. Tsirkin - Sept. 10, 2009, 6:03 p.m.
On Thu, Sep 10, 2009 at 04:18:45PM +0100, Mark McLoughlin wrote:
> qemu_opt_set() prints an error message in all failure cases, so
> qemu_set_option() doesn't need to print another error.
> 
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>

Same comment about {} here

> ---
>  qemu-config.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/qemu-config.c b/qemu-config.c
> index 61e6ed1..733cc24 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -186,11 +186,9 @@ int qemu_set_option(const char *str)
>          return -1;
>      }
>  
> -    if (qemu_opt_set(opts, arg, str+offset+1) == -1) {
> -        fprintf(stderr, "failed to set \"%s\" for %s \"%s\"\n",
> -                arg, lists[i]->name, id);
> +    if (qemu_opt_set(opts, arg, str+offset+1) == -1)
>          return -1;
> -    }
> +
>      return 0;
>  }
>  
> -- 
> 1.6.2.5
> 
>
Mark McLoughlin - Sept. 10, 2009, 6:36 p.m.
On Thu, 2009-09-10 at 21:03 +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 10, 2009 at 04:18:45PM +0100, Mark McLoughlin wrote:
> > qemu_opt_set() prints an error message in all failure cases, so
> > qemu_set_option() doesn't need to print another error.
> > 
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> 
> Same comment about {} here

That's 3 of these you and Juan have found. And, genuinely, I try not to
do this. Just shows how much of reflex it is.

IMHO,

  if (blaa(foo, bar, doodah) == NULL)
      return -1;

is far nicer than:

  if (blaa(foo, bar, doodah) == NULL) {
      return -1;
  }

but OTOH, this:

  if (blaa(foo, bar, doodah) == NULL) {
      return doodah(foo, bar, blaa);
  }

*is* arguably better than:

  if (blaa(foo, bar, doodah) == NULL)
      return doodah(foo, bar, blaa);

There's enough of both ways in the code that I think either should be
acceptable.

Cheers,
Mark.
Michael S. Tsirkin - Sept. 11, 2009, 5:01 a.m.
On Thu, Sep 10, 2009 at 07:36:09PM +0100, Mark McLoughlin wrote:
> On Thu, 2009-09-10 at 21:03 +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 10, 2009 at 04:18:45PM +0100, Mark McLoughlin wrote:
> > > qemu_opt_set() prints an error message in all failure cases, so
> > > qemu_set_option() doesn't need to print another error.
> > > 
> > > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> > 
> > Same comment about {} here
> 
> That's 3 of these you and Juan have found. And, genuinely, I try not to
> do this. Just shows how much of reflex it is.
> 
> IMHO,
> 
>   if (blaa(foo, bar, doodah) == NULL)
>       return -1;
> 
> is far nicer than:
> 
>   if (blaa(foo, bar, doodah) == NULL) {
>       return -1;
>   }
> 
> but OTOH, this:
> 
>   if (blaa(foo, bar, doodah) == NULL) {
>       return doodah(foo, bar, blaa);
>   }
> 
> *is* arguably better than:
> 
>   if (blaa(foo, bar, doodah) == NULL)
>       return doodah(foo, bar, blaa);

And 
  if (!blaa(foo, bar, doodah))
is even better :)

> There's enough of both ways in the code that I think either should be
> acceptable.
> 
> Cheers,
> Mark.
Amit Shah - Sept. 11, 2009, 5:50 a.m.
On (Thu) Sep 10 2009 [19:36:09], Mark McLoughlin wrote:
> On Thu, 2009-09-10 at 21:03 +0300, Michael S. Tsirkin wrote:
> > On Thu, Sep 10, 2009 at 04:18:45PM +0100, Mark McLoughlin wrote:
> > > qemu_opt_set() prints an error message in all failure cases, so
> > > qemu_set_option() doesn't need to print another error.
> > > 
> > > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> > 
> > Same comment about {} here
> 
> That's 3 of these you and Juan have found. And, genuinely, I try not to
> do this. Just shows how much of reflex it is.
> 
> IMHO,
> 
>   if (blaa(foo, bar, doodah) == NULL)
>       return -1;
> 
> is far nicer than:
> 
>   if (blaa(foo, bar, doodah) == NULL) {
>       return -1;
>   }

BTW in patch 1 of the series you convert

    if (foo == NULL)

to

    if (!foo)

so maybe you'd want to be consistent in the rest of the series as well?

		Amit

Patch

diff --git a/qemu-config.c b/qemu-config.c
index 61e6ed1..733cc24 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -186,11 +186,9 @@  int qemu_set_option(const char *str)
         return -1;
     }
 
-    if (qemu_opt_set(opts, arg, str+offset+1) == -1) {
-        fprintf(stderr, "failed to set \"%s\" for %s \"%s\"\n",
-                arg, lists[i]->name, id);
+    if (qemu_opt_set(opts, arg, str+offset+1) == -1)
         return -1;
-    }
+
     return 0;
 }