<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 23, 2021 at 9:50 AM Christian MAUDERER <<a href="mailto:christian.mauderer@embedded-brains.de">christian.mauderer@embedded-brains.de</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello Joel,<br>
<br>
Am 23.02.21 um 16:23 schrieb Joel Sherrill:<br>
> Hi<br>
> <br>
> Ryan has wandered into the land of third party code and Coverity issues. <br>
> It is not a very welcoming land and we need to decide what we want to do <br>
> as a project. I put one<br>
> <br>
> In these cases, we have a few patterns to fall back on. There are <br>
> basically a few choices here:<br>
> <br>
> + ignore third party code in Coverity. I'm not a fan since we still have <br>
> code with issues.<br>
<br>
Depends a bit on the third party code and the types of Coverity errors. <br>
But in general you are right: It's not a good solution.<br></blockquote><div><br></div><div>There isn't a good one.  </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> <br>
>    + Do the minimum which is often just adding (void) in front of the <br>
> call not checking a return code..Still wrap in #ifdef __rtems__ as below <br>
> with a more robust solution.<br>
> <br>
> + Add #ifdef __rtems__ so the original code and rtems changes are <br>
> side by side. This is more like what we do with libbsd. But I would <br>
> still tend to use the correct fix and not just slap a (void) in front. <br>
> If we are going to modify the file, we might as well fix it.<br>
> <br>
> #ifdef __rtems__<br>
>     rc = XXX<br>
>     _Assert_Unused_variable_equals'<br>
> #else<br>
>     XXX<br>
> #endif<br>
> <br>
> Any other options? Opinions.<br>
<br>
Best option would be to fix it upstream (regardless whether it's a real <br>
bug or a false positive) and update our copy of the code.<br>
<br>
If that is not an option (patches not accepted; running out of time; <br>
some other reason) I think it needs a case by case analysis:<br>
<br>
Is it really a potential problem or is it a false positive of Coverity.<br>
<br>
1) If it is a false positive we have two conflicting interests:<br>
<br>
a) Have as few changes as possible so that we can easily upgrade third <br>
party code to newer versions.<br>
<br>
b) Have as few Coverity warnings as possible.<br>
<br>
If possible I would rather disable specific warnings for specific third <br>
party files (or on a function level - is that possible?) rather then <br>
adding complexity to upgrades. Otherwise we might tend to rather not do <br>
an upgrade because it's too much work to re-integrate all the patches.<br></blockquote><div><br></div><div>This isn't possible but we can flag a specific case as "don't care" and</div><div>move on. </div><div><br></div><div>I'm prone to think the one in the patch below is OK to do this with </div><div>since even if the calls fails, the next call to fdt_next_tag() will also </div><div>fail and get caught. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
2) If it is not a false positive it should be definitively fixed (and <br>
not only masked with a (void)). In that case I think the #if(n)def <br>
__rtems__ like in libbsd is a good solution to make updates as simple as <br>
possible.<br></blockquote><div><br></div><div>I suppose this is the solution and trying to at least file an issue upstream.</div><div><br></div><div>I'm hesitant to hold up fixing it for us while waiting on a fix upstream and then</div><div>an update on our side is twofold. First, the loop is quite time consuming even</div><div>when it works. Worse, my history with upstreams and Coverity issues isn't great. </div><div>It is possible in the end that we get stuck with the issue after all. I had this happen</div><div>with gzip where the end result was an email that said they had decided it was a</div><div>false positive. But every project including gzip gets the same issue flagged. :(</div><div><br></div><div>We'll try to be good open source citizens, file issues upstream, and do what we</div><div>think is right locally.</div><div><br></div><div>--joel</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Best regards<br>
<br>
Christian<br>
<br>
> <br>
> --joel<br>
> <br>
> ============================<br>
>   CID 1047324: Unchecked return value in fdt_add_subnode_namelen().<br>
> <br>
> Closes #4256<br>
> ---<br>
> cpukit/dtc/libfdt/fdt_rw.c | 2 +-<br>
> 1 file changed, 1 insertion(+), 1 deletion(-)<br>
> <br>
> diff --git a/cpukit/dtc/libfdt/fdt_rw.c b/cpukit/dtc/libfdt/fdt_rw.c<br>
> index 1385425..ceeeb44 100644<br>
> --- a/cpukit/dtc/libfdt/fdt_rw.c<br>
> +++ b/cpukit/dtc/libfdt/fdt_rw.c<br>
> @@ -348,7 +348,7 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset,<br>
> return offset;<br>
> /* Try to place the new node after the parent's properties */<br>
> - fdt_next_tag(fdt, parentoffset, &nextoffset); /* skip the BEGIN_NODE */<br>
> + (void)fdt_next_tag(fdt, parentoffset, &nextoffset); /* skip the <br>
> BEGIN_NODE */<br>
> do {<br>
> offset = nextoffset;<br>
> tag = fdt_next_tag(fdt, offset, &nextoffset);<br>
> -- <br>
> 1.8.3.1<br>
> <br>
> _______________________________________________<br>
> devel mailing list<br>
> <a href="mailto:devel@rtems.org" target="_blank">devel@rtems.org</a><br>
> <a href="http://lists.rtems.org/mailman/listinfo/devel" rel="noreferrer" target="_blank">http://lists.rtems.org/mailman/listinfo/devel</a><br>
> <br>
<br>
-- <br>
--------------------------------------------<br>
embedded brains GmbH<br>
Herr Christian MAUDERER<br>
Dornierstr. 4<br>
82178 Puchheim<br>
Germany<br>
email: <a href="mailto:christian.mauderer@embedded-brains.de" target="_blank">christian.mauderer@embedded-brains.de</a><br>
phone: +49-89-18 94 741 - 18<br>
fax:   +49-89-18 94 741 - 08<br>
<br>
Registergericht: Amtsgericht München<br>
Registernummer: HRB 157899<br>
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler<br>
Unsere Datenschutzerklärung finden Sie hier:<br>
<a href="https://embedded-brains.de/datenschutzerklaerung/" rel="noreferrer" target="_blank">https://embedded-brains.de/datenschutzerklaerung/</a><br>
</blockquote></div></div>