Patchwork qemu-ga: Remove unreachable code after g_error

login
register
mail settings
Submitter Stefan Weil
Date Sept. 1, 2012, 7:34 a.m.
Message ID <1346484855-29649-1-git-send-email-sw@weilnetz.de>
Download mbox | patch
Permalink /patch/181123/
State Accepted
Headers show

Comments

Stefan Weil - Sept. 1, 2012, 7:34 a.m.
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(-)
Luiz Capitulino - Sept. 3, 2012, 4:49 p.m.
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;
Stefan Weil - Sept. 3, 2012, 5:02 p.m.
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
Luiz Capitulino - Sept. 3, 2012, 5:57 p.m.
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.
Stefan Hajnoczi - Sept. 22, 2012, 3:25 p.m.
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

Patch

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;