| Message ID | 1252595941-15196-4-git-send-email-markmc@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
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 > >
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.
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.
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
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; }
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(-)