| Message ID | 1346484855-29649-1-git-send-email-sw@weilnetz.de |
|---|---|
| State | Accepted |
| Headers | show |
On Sat, 1 Sep 2012 09:34:15 +0200 Stefan Weil <sw@weilnetz.de> wrote: > Report from smatch: > qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code. > qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code. > > g_error calls abort which terminates the program. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > qemu-ga.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/qemu-ga.c b/qemu-ga.c > index 7623079..b747470 100644 > --- a/qemu-ga.c > +++ b/qemu-ga.c > @@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void) > ret = sigaction(SIGINT, &sigact, NULL); > if (ret == -1) { > g_error("error configuring signal handler: %s", strerror(errno)); > - return false; Good catch, but we should really drop g_error() usage as qemu-ga will not fail gracefully otherwise (will leak the pidfile, for example). We either just drop g_error() or replace it by fprintf(). > } > ret = sigaction(SIGTERM, &sigact, NULL); > if (ret == -1) { > g_error("error configuring signal handler: %s", strerror(errno)); > - return false; > } > > return true;
Am 03.09.2012 18:49, schrieb Luiz Capitulino: > On Sat, 1 Sep 2012 09:34:15 +0200 > Stefan Weil <sw@weilnetz.de> wrote: > >> Report from smatch: >> qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code. >> qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code. >> >> g_error calls abort which terminates the program. >> >> Signed-off-by: Stefan Weil <sw@weilnetz.de> >> --- >> qemu-ga.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/qemu-ga.c b/qemu-ga.c >> index 7623079..b747470 100644 >> --- a/qemu-ga.c >> +++ b/qemu-ga.c >> @@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void) >> ret = sigaction(SIGINT, &sigact, NULL); >> if (ret == -1) { >> g_error("error configuring signal handler: %s", strerror(errno)); >> - return false; > Good catch, but we should really drop g_error() usage as qemu-ga will not > fail gracefully otherwise (will leak the pidfile, for example). We either > just drop g_error() or replace it by fprintf(). Isn't that a classical case of an error which should never occur, something which could also be handled by an assert statement? I don't expect a graceful exit after such errors. If they occur, that's something which must be fixed in the code. When I read the documentation of sigaction, I don't see how it could fail with the given function arguments. Therefore I'd apply the patch as it is. Regards, - sw
On Mon, 03 Sep 2012 19:02:20 +0200 Stefan Weil <sw@weilnetz.de> wrote: > Am 03.09.2012 18:49, schrieb Luiz Capitulino: > > On Sat, 1 Sep 2012 09:34:15 +0200 > > Stefan Weil <sw@weilnetz.de> wrote: > > > >> Report from smatch: > >> qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code. > >> qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code. > >> > >> g_error calls abort which terminates the program. > >> > >> Signed-off-by: Stefan Weil <sw@weilnetz.de> > >> --- > >> qemu-ga.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/qemu-ga.c b/qemu-ga.c > >> index 7623079..b747470 100644 > >> --- a/qemu-ga.c > >> +++ b/qemu-ga.c > >> @@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void) > >> ret = sigaction(SIGINT, &sigact, NULL); > >> if (ret == -1) { > >> g_error("error configuring signal handler: %s", strerror(errno)); > >> - return false; > > Good catch, but we should really drop g_error() usage as qemu-ga will not > > fail gracefully otherwise (will leak the pidfile, for example). We either > > just drop g_error() or replace it by fprintf(). > > Isn't that a classical case of an error which should never occur, > something which could also be handled by an assert statement? > > I don't expect a graceful exit after such errors. If they occur, > that's something which must be fixed in the code. > > When I read the documentation of sigaction, I don't see how > it could fail with the given function arguments. > > Therefore I'd apply the patch as it is. Yes, taking a look at the sigaction() manpages shows you're obviously correct. Please, disregard what I've said.
On Sat, Sep 01, 2012 at 09:34:15AM +0200, Stefan Weil wrote: > Report from smatch: > qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code. > qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code. > > g_error calls abort which terminates the program. > > Signed-off-by: Stefan Weil <sw@weilnetz.de> > --- > qemu-ga.c | 2 -- > 1 file changed, 2 deletions(-) Thanks, applied to the trivial patches tree: https://github.com/stefanha/qemu/commits/trivial-patches Stefan
diff --git a/qemu-ga.c b/qemu-ga.c index 7623079..b747470 100644 --- a/qemu-ga.c +++ b/qemu-ga.c @@ -114,12 +114,10 @@ static gboolean register_signal_handlers(void) ret = sigaction(SIGINT, &sigact, NULL); if (ret == -1) { g_error("error configuring signal handler: %s", strerror(errno)); - return false; } ret = sigaction(SIGTERM, &sigact, NULL); if (ret == -1) { g_error("error configuring signal handler: %s", strerror(errno)); - return false; } return true;
Report from smatch: qemu-ga.c:117 register_signal_handlers(11) info: ignoring unreachable code. qemu-ga.c:122 register_signal_handlers(16) info: ignoring unreachable code. g_error calls abort which terminates the program. Signed-off-by: Stefan Weil <sw@weilnetz.de> --- qemu-ga.c | 2 -- 1 file changed, 2 deletions(-)