Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI: add scheduled Coverity scan #439

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chipitsine
Copy link
Contributor

No description provided.

Signed-off-by: Ilya Shipitsin <chipitsine@gmail.com>
@paulusmack
Copy link
Collaborator

@RICCIARDI-Adrien @enaess any comments on this PR?

@Neustradamus
Copy link
Member

@paulusmack, @chipitsine: "--form email=chipitsine@gmail.com".

@chipitsine
Copy link
Contributor Author

email is required field.
what email do you prefer to specify ?

@RICCIARDI-Adrien
Copy link
Contributor

@chipitsine Even with another email address, will it be possible for people not having access to the email account to get access to the Coverity reports ?

@chipitsine
Copy link
Contributor Author

@chipitsine Even with another email address, will it be possible for people not having access to the email account to get access to the Coverity reports ?

sure.

@chipitsine
Copy link
Contributor Author

@RICCIARDI-Adrien , even now, scheduled scan is not available yet, I ran scan manually.
if you have capasity/will to review findings, I can add you account

image

@RICCIARDI-Adrien
Copy link
Contributor

I'm not fond of a solution that is not fully open source, so maybe we can add continue-on-error: true (see https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idcontinue-on-error) to the job, so the whole CI is not blocked if something goes wrong with this specific job (for instance, the free license changes).

@chipitsine
Copy link
Contributor Author

chipitsine commented Oct 2, 2023 via email

@RICCIARDI-Adrien
Copy link
Contributor

Oh yes, you are correct, forget my comment !

@paulusmack
Copy link
Collaborator

So, what happens if Coverity decides that there is an error? Just an email to somewhere, or what?

@chipitsine
Copy link
Contributor Author

coverity has nice web UI

image

image

image

it is already setup, I can add you to project (I need your email).

project members can browse issue in UI, also email is sent to them on new issues

@paulusmack
Copy link
Collaborator

project members can browse issue in UI, also email is sent to them on new issues

Please add me, paulus@ozlabs.org.

How are the coverity scans done today? Do you do some manual step to trigger a scan, or something?

What are our options in the situations where coverity says something is an error but in fact it's correct? We have seen such situations in the past.

@chipitsine
Copy link
Contributor Author

project members can browse issue in UI, also email is sent to them on new issues

Please add me, paulus@ozlabs.org.

How are the coverity scans done today? Do you do some manual step to trigger a scan, or something?

What are our options in the situations where coverity says something is an error but in fact it's correct? We have seen such situations in the past.

there are few ways.

  1. you can mark finding as "intentional" or "false positive"

image

  1. alternatively, false positive may be marked in code comments https://community.synopsys.com/s/article/Suppressing-False-Positive-Intentional-defects

  2. or via model https://community.synopsys.com/s/article/How-to-write-a-function-model-to-eliminate-a-false-positive-in-a-C-applilcation

@chipitsine
Copy link
Contributor Author

project members can browse issue in UI, also email is sent to them on new issues

Please add me, paulus@ozlabs.org.

How are the coverity scans done today? Do you do some manual step to trigger a scan, or something?

I've created CI task in my fork: https://github.com/chipitsine/ppp/tree/coverity
I merge master branch from time to time

@chipitsine
Copy link
Contributor Author

project members can browse issue in UI, also email is sent to them on new issues

Please add me, paulus@ozlabs.org.

I sent an invitation (maybe it reached spam folder)

image

@paulusmack
Copy link
Collaborator

The invitation did go to spam, but I found it.

Seems like coverity is complaining nearly every time we use warn() (or info(), fatal(), error(), etc.) because of the existence of implementations of those functions in pppd/plugins/pppoe/pppoe-discovery.c, which of course are not the implementations that get compiled into pppd. I guess it has no understanding of makefiles and just blindly assumes all source files are compiled into one big mess.

@chipitsine
Copy link
Contributor Author

The invitation did go to spam, but I found it.

Seems like coverity is complaining nearly every time we use warn() (or info(), fatal(), error(), etc.) because of the existence of implementations of those functions in pppd/plugins/pppoe/pppoe-discovery.c, which of course are not the implementations that get compiled into pppd. I guess it has no understanding of makefiles and just blindly assumes all source files are compiled into one big mess.

it does not make any assumption.
it just follows the build process

image

@paulusmack
Copy link
Collaborator

it does not make any assumption. it just follows the build process

So why does it think that the warn() called from various places in ipcp.c could ever be the warn() defined in pppoe-discovery.c? Those two files are never compiled into the same executable. The calls in ipcp.c are to the warn() defined in utils.c. But when you look at the coverity errors, it is clearly linking the call in ipcp.c to the function in pppoe-discovery.c.

@chipitsine
Copy link
Contributor Author

chipitsine commented Oct 16, 2023

there's non zero probabilty that "ppp" configures itself to use plugins from pppoe folder.

I ran steps from CI, in folder "pppd" I see:

~/ppp/pppd$ grep -i pppoe Makefile | grep -i include
DEFAULT_INCLUDES = -I. -I$(top_builddir)/pppd/plugins/pppoe
~/ppp/pppd$ 

@paulusmack
Copy link
Collaborator

there's non zero probabilty that "ppp" configures itself to use plugins from pppoe folder.

Sure... and?

The point is that coverity is putting together a use of a function (in this case, warn()) in pppd with a definition which is not part of pppd or the pppoe plugin. To say it a different way, the code in pppoe-discovery.c (which contains an implementation of warn()) never appears in the address space of a pppd process. It is not part of pppd and it is not part of the pppoe plugin. What it is, is part of a separate standalone program which happens to use some of the same source code as the pppoe plugin (but note that the shared code does not include pppoe-discovery.c).

The result of coverity not understanding what gets linked with what is that it complains about things that are perfectly fine, making it quite hard to see what things do actually need attention.

I have no problem with coverity combining the code for pppd with the code for the pppoe plugin. But not all the source code in the pppd/plugins/pppoe directory goes into the pppoe plugin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants