Re: [PATCH] cleanup coverity findging (make it silent)
well, I do not have an idea why extchk_setenv(check, EXTCHK_HAPROXY_SERVER_ADDR, check->argv[3]); is used instead of EXTCHK_SETENV(check, EXTCHK_HAPROXY_SERVER_ADDR, check->argv[3], err); it means, some environment variables are set in "best effort" mode, i.e. error is ignored. is it bad ? I'm not sure. code comes from https://github.com/haproxy/haproxy/commit/61cc85223098a962616ececa2d6bdd7809c37fe3 Christopher, do you know why we ignore exit status here ? вт, 26 мая 2020 г. в 19:59, Илья Шипицин : > > > вт, 26 мая 2020 г. в 12:02, Willy Tarreau : > >> Hi Ilya, >> >> On Sat, May 23, 2020 at 03:47:58PM +0500, ??? wrote: >> > From: Ilya Shipitsin >> > Date: Sat, 23 May 2020 15:35:36 +0500 >> > Subject: [PATCH] CLEANUP: src/checks.c: ignore return value using >> DISGUISE(..) >> > >> > we do not want to check status of extchk_setenv, but static analyzsers >> > like Coverity are not happy about it. Let calm coverity down. >> >> Are you really sure we don't want to check them ? I'm seeing that >> prepare_external_check() uses EXTCHK_SETENV() to purposely add checks >> there, so it's unclear to me why we want to silently fail here. Maybe >> the calls should instead be changed to have a check and a jump to an >> error label doing the exit(). >> >> I don't know if anyone has an opinion on this, I'm not using external >> checks :-/ >> > > well, I meant to keep current behaviour, but also silence coverity warning. > > ok, we can investigate and discuss would it be better to change current > behaviour or to keep it. > > >> >> Willy >> >
Re: [PATCH] cleanup coverity findging (make it silent)
вт, 26 мая 2020 г. в 12:02, Willy Tarreau : > Hi Ilya, > > On Sat, May 23, 2020 at 03:47:58PM +0500, ??? wrote: > > From: Ilya Shipitsin > > Date: Sat, 23 May 2020 15:35:36 +0500 > > Subject: [PATCH] CLEANUP: src/checks.c: ignore return value using > DISGUISE(..) > > > > we do not want to check status of extchk_setenv, but static analyzsers > > like Coverity are not happy about it. Let calm coverity down. > > Are you really sure we don't want to check them ? I'm seeing that > prepare_external_check() uses EXTCHK_SETENV() to purposely add checks > there, so it's unclear to me why we want to silently fail here. Maybe > the calls should instead be changed to have a check and a jump to an > error label doing the exit(). > > I don't know if anyone has an opinion on this, I'm not using external > checks :-/ > well, I meant to keep current behaviour, but also silence coverity warning. ok, we can investigate and discuss would it be better to change current behaviour or to keep it. > > Willy >
Re: [PATCH] cleanup coverity findging (make it silent)
Hi Ilya, On Sat, May 23, 2020 at 03:47:58PM +0500, ??? wrote: > From: Ilya Shipitsin > Date: Sat, 23 May 2020 15:35:36 +0500 > Subject: [PATCH] CLEANUP: src/checks.c: ignore return value using DISGUISE(..) > > we do not want to check status of extchk_setenv, but static analyzsers > like Coverity are not happy about it. Let calm coverity down. Are you really sure we don't want to check them ? I'm seeing that prepare_external_check() uses EXTCHK_SETENV() to purposely add checks there, so it's unclear to me why we want to silently fail here. Maybe the calls should instead be changed to have a check and a jump to an error label doing the exit(). I don't know if anyone has an opinion on this, I'm not using external checks :-/ Willy
[PATCH] cleanup coverity findging (make it silent)
Hello, let us clean up non important finding by wrapping it with DISGUISE(..) Cheers, Ilya Shipitcin From 7060a886a76452245ec466f6f7aaf28d504c9c3f Mon Sep 17 00:00:00 2001 From: Ilya Shipitsin Date: Sat, 23 May 2020 15:35:36 +0500 Subject: [PATCH] CLEANUP: src/checks.c: ignore return value using DISGUISE(..) we do not want to check status of extchk_setenv, but static analyzsers like Coverity are not happy about it. Let calm coverity down. --- src/checks.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/checks.c b/src/checks.c index 01a27f87e..ea99145bd 100644 --- a/src/checks.c +++ b/src/checks.c @@ -3208,15 +3208,15 @@ static int connect_proc_chk(struct task *t) environ = check->envp; /* Update some environment variables and command args: curconn, server addr and server port */ - extchk_setenv(check, EXTCHK_HAPROXY_SERVER_CURCONN, ultoa_r(s->cur_sess, buf, sizeof(buf))); + DISGUISE(extchk_setenv(check, EXTCHK_HAPROXY_SERVER_CURCONN, ultoa_r(s->cur_sess, buf, sizeof(buf; addr_to_str(&s->addr, check->argv[3], EXTCHK_SIZE_ADDR); - extchk_setenv(check, EXTCHK_HAPROXY_SERVER_ADDR, check->argv[3]); + DISGUISE(extchk_setenv(check, EXTCHK_HAPROXY_SERVER_ADDR, check->argv[3])); *check->argv[4] = 0; if (s->addr.ss_family == AF_INET || s->addr.ss_family == AF_INET6) snprintf(check->argv[4], EXTCHK_SIZE_UINT, "%u", s->svc_port); - extchk_setenv(check, EXTCHK_HAPROXY_SERVER_PORT, check->argv[4]); + DISGUISE(extchk_setenv(check, EXTCHK_HAPROXY_SERVER_PORT, check->argv[4])); haproxy_unblock_signals(); execvp(px->check_command, check->argv); -- 2.26.2
