* [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown()
@ 2021-09-01 8:45 Jan Beulich
2021-09-01 8:49 ` Bertrand Marquis
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jan Beulich @ 2021-09-01 8:45 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu
Commit 806448806264 ("xen/domain: Fix label position in
domain_teardown()" has caused Coverity to report a _new_ supposedly
un-annotated fall-through in a switch(). I find this (once again)
puzzling; I'm having an increasingly hard time figuring what patterns
the tool is actually after. I would have expected that the tool would
either have spotted an issue also before this change, or not at all. Yet
if it had spotted one before, the statistics report should have included
an eliminated instance alongside the new one (because then the issue
would simply have moved by a few lines).
Hence the only thing I could guess is that the treatment of comments in
macro expansions might be subtly different. Therefore try whether
switching the comments to the still relatively new "fallthrough" pseudo
keyword actually helps.
Coverity-ID: 1490865
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
If this doesn't help, I'm afraid I'm lost as to what Coverity means us
to do to silence the reporting.
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -401,13 +401,13 @@ static int domain_teardown(struct domain
*/
#define PROGRESS(x) \
d->teardown.val = PROG_ ## x; \
- /* Fallthrough */ \
+ fallthrough; \
case PROG_ ## x
#define PROGRESS_VCPU(x) \
d->teardown.val = PROG_vcpu_ ## x; \
d->teardown.vcpu = v; \
- /* Fallthrough */ \
+ fallthrough; \
case PROG_vcpu_ ## x: \
v = d->teardown.vcpu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown()
2021-09-01 8:45 [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown() Jan Beulich
@ 2021-09-01 8:49 ` Bertrand Marquis
2021-09-01 8:53 ` Jan Beulich
2021-09-03 16:31 ` Julien Grall
2021-09-08 10:32 ` Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Bertrand Marquis @ 2021-09-01 8:49 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu
Hi Jan,
> On 1 Sep 2021, at 09:45, Jan Beulich <jbeulich@suse.com> wrote:
>
> Commit 806448806264 ("xen/domain: Fix label position in
> domain_teardown()" has caused Coverity to report a _new_ supposedly
> un-annotated fall-through in a switch(). I find this (once again)
> puzzling; I'm having an increasingly hard time figuring what patterns
> the tool is actually after. I would have expected that the tool would
> either have spotted an issue also before this change, or not at all. Yet
> if it had spotted one before, the statistics report should have included
> an eliminated instance alongside the new one (because then the issue
> would simply have moved by a few lines).
>
> Hence the only thing I could guess is that the treatment of comments in
> macro expansions might be subtly different. Therefore try whether
> switching the comments to the still relatively new "fallthrough" pseudo
> keyword actually helps.
>
> Coverity-ID: 1490865
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
A grep inside Xen code show that we have occurence of:
/* fallthrough */
/* Fallthrough */
falltrough
Should we actually fix all of them ?
Anyway this can be in an other patch.
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Cheers
Bertrand
> ---
> If this doesn't help, I'm afraid I'm lost as to what Coverity means us
> to do to silence the reporting.
>
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -401,13 +401,13 @@ static int domain_teardown(struct domain
> */
> #define PROGRESS(x) \
> d->teardown.val = PROG_ ## x; \
> - /* Fallthrough */ \
> + fallthrough; \
> case PROG_ ## x
>
> #define PROGRESS_VCPU(x) \
> d->teardown.val = PROG_vcpu_ ## x; \
> d->teardown.vcpu = v; \
> - /* Fallthrough */ \
> + fallthrough; \
> case PROG_vcpu_ ## x: \
> v = d->teardown.vcpu
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown()
2021-09-01 8:49 ` Bertrand Marquis
@ 2021-09-01 8:53 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2021-09-01 8:53 UTC (permalink / raw)
To: Bertrand Marquis
Cc: xen-devel, Andrew Cooper, George Dunlap, Ian Jackson,
Julien Grall, Stefano Stabellini, Wei Liu
On 01.09.2021 10:49, Bertrand Marquis wrote:
>> On 1 Sep 2021, at 09:45, Jan Beulich <jbeulich@suse.com> wrote:
>> Commit 806448806264 ("xen/domain: Fix label position in
>> domain_teardown()" has caused Coverity to report a _new_ supposedly
>> un-annotated fall-through in a switch(). I find this (once again)
>> puzzling; I'm having an increasingly hard time figuring what patterns
>> the tool is actually after. I would have expected that the tool would
>> either have spotted an issue also before this change, or not at all. Yet
>> if it had spotted one before, the statistics report should have included
>> an eliminated instance alongside the new one (because then the issue
>> would simply have moved by a few lines).
>>
>> Hence the only thing I could guess is that the treatment of comments in
>> macro expansions might be subtly different. Therefore try whether
>> switching the comments to the still relatively new "fallthrough" pseudo
>> keyword actually helps.
>>
>> Coverity-ID: 1490865
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> A grep inside Xen code show that we have occurence of:
> /* fallthrough */
> /* Fallthrough */
> falltrough
>
> Should we actually fix all of them ?
I don't think so, or at least don't view this as urgent. As described
what I'm suspecting here is an issue with such comments living inside
macros. I don't think we have too many instances of such. In case my
guess is right (and hence in case the patch actually helps), we may
want to change all in this much smaller set, but as you say ...
> Anyway this can be in an other patch.
... in a separate patch (not the least because we still need to see
whether the change helps in the first place).
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
Thanks.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown()
2021-09-01 8:45 [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown() Jan Beulich
2021-09-01 8:49 ` Bertrand Marquis
@ 2021-09-03 16:31 ` Julien Grall
2021-09-08 10:32 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Julien Grall @ 2021-09-03 16:31 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Stefano Stabellini, Wei Liu
Hi Jan,
On 01/09/2021 09:45, Jan Beulich wrote:
> Commit 806448806264 ("xen/domain: Fix label position in
> domain_teardown()" has caused Coverity to report a _new_ supposedly
> un-annotated fall-through in a switch(). I find this (once again)
> puzzling; I'm having an increasingly hard time figuring what patterns
> the tool is actually after. I would have expected that the tool would
> either have spotted an issue also before this change, or not at all. Yet
> if it had spotted one before, the statistics report should have included
> an eliminated instance alongside the new one (because then the issue
> would simply have moved by a few lines).
>
> Hence the only thing I could guess is that the treatment of comments in
> macro expansions might be subtly different. Therefore try whether
> switching the comments to the still relatively new "fallthrough" pseudo
> keyword actually helps.
>
> Coverity-ID: 1490865
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <jgrall@amazon.com>
> ---
> If this doesn't help, I'm afraid I'm lost as to what Coverity means us
> to do to silence the reporting.
>
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -401,13 +401,13 @@ static int domain_teardown(struct domain
> */
> #define PROGRESS(x) \
> d->teardown.val = PROG_ ## x; \
> - /* Fallthrough */ \
> + fallthrough; \
> case PROG_ ## x
>
> #define PROGRESS_VCPU(x) \
> d->teardown.val = PROG_vcpu_ ## x; \
> d->teardown.vcpu = v; \
> - /* Fallthrough */ \
> + fallthrough; \
> case PROG_vcpu_ ## x: \
> v = d->teardown.vcpu
>
>
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown()
2021-09-01 8:45 [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown() Jan Beulich
2021-09-01 8:49 ` Bertrand Marquis
2021-09-03 16:31 ` Julien Grall
@ 2021-09-08 10:32 ` Jan Beulich
2021-09-08 12:34 ` Jan Beulich
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2021-09-08 10:32 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu
On 01.09.2021 10:45, Jan Beulich wrote:
> Commit 806448806264 ("xen/domain: Fix label position in
> domain_teardown()" has caused Coverity to report a _new_ supposedly
> un-annotated fall-through in a switch(). I find this (once again)
> puzzling; I'm having an increasingly hard time figuring what patterns
> the tool is actually after. I would have expected that the tool would
> either have spotted an issue also before this change, or not at all. Yet
> if it had spotted one before, the statistics report should have included
> an eliminated instance alongside the new one (because then the issue
> would simply have moved by a few lines).
>
> Hence the only thing I could guess is that the treatment of comments in
> macro expansions might be subtly different. Therefore try whether
> switching the comments to the still relatively new "fallthrough" pseudo
> keyword actually helps.
>
> Coverity-ID: 1490865
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> If this doesn't help, I'm afraid I'm lost as to what Coverity means us
> to do to silence the reporting.
According to the most recent report this did not help. Shall I
revert the change? Or do we consider it a step towards using the
pseudo keyword more uniformly?
Jan
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -401,13 +401,13 @@ static int domain_teardown(struct domain
> */
> #define PROGRESS(x) \
> d->teardown.val = PROG_ ## x; \
> - /* Fallthrough */ \
> + fallthrough; \
> case PROG_ ## x
>
> #define PROGRESS_VCPU(x) \
> d->teardown.val = PROG_vcpu_ ## x; \
> d->teardown.vcpu = v; \
> - /* Fallthrough */ \
> + fallthrough; \
> case PROG_vcpu_ ## x: \
> v = d->teardown.vcpu
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown()
2021-09-08 10:32 ` Jan Beulich
@ 2021-09-08 12:34 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2021-09-08 12:34 UTC (permalink / raw)
To: xen-devel
Cc: Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu
On 08.09.2021 12:32, Jan Beulich wrote:
> On 01.09.2021 10:45, Jan Beulich wrote:
>> Commit 806448806264 ("xen/domain: Fix label position in
>> domain_teardown()" has caused Coverity to report a _new_ supposedly
>> un-annotated fall-through in a switch(). I find this (once again)
>> puzzling; I'm having an increasingly hard time figuring what patterns
>> the tool is actually after. I would have expected that the tool would
>> either have spotted an issue also before this change, or not at all. Yet
>> if it had spotted one before, the statistics report should have included
>> an eliminated instance alongside the new one (because then the issue
>> would simply have moved by a few lines).
>>
>> Hence the only thing I could guess is that the treatment of comments in
>> macro expansions might be subtly different. Therefore try whether
>> switching the comments to the still relatively new "fallthrough" pseudo
>> keyword actually helps.
>>
>> Coverity-ID: 1490865
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> If this doesn't help, I'm afraid I'm lost as to what Coverity means us
>> to do to silence the reporting.
>
> According to the most recent report this did not help.
Just noticed that this was rubbish: I thought I had committed the change,
but I actually didn't. Now I'm about to - let's see what happens.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-08 12:34 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 8:45 [PATCH] domain: try to address Coverity pointing out a missing "break" in domain_teardown() Jan Beulich
2021-09-01 8:49 ` Bertrand Marquis
2021-09-01 8:53 ` Jan Beulich
2021-09-03 16:31 ` Julien Grall
2021-09-08 10:32 ` Jan Beulich
2021-09-08 12:34 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.