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

Fix a few issues found by scanner #1689

Merged
merged 3 commits into from
Dec 15, 2023
Merged

Fix a few issues found by scanner #1689

merged 3 commits into from
Dec 15, 2023

Conversation

preichl
Copy link
Contributor

@preichl preichl commented Dec 15, 2023

Following issues were found by Red Hat's OpenScanHub:

fio-3.35/engines/rdma.c:279:3: warning[deadcode.DeadStores]:
Value stored to 'ret' is never read

fio-3.35/server.c:1884:3: warning[deadcode.DeadStores]:
Value stored to 'extended_buf_wp' is never read

fio-3.35/engines/http.c:642: var_tested_neg:
Assigning: "r" = a negative value.
fio-3.35/engines/http.c:714: assign: Assigning: "io_u->error" = "r". fio-3.35/engines/http.c:715: negative_returns: "io_u->error" is passed
to a parameter that cannot be negative.

I suggest changing the data type of io_u->error from uint to int since it appears that negative values are being assigned to it.

@axboe
Copy link
Owner

axboe commented Dec 15, 2023

The changes looks fine, but the commit title:

Fix a few issues found by scanner

is pretty bad as it means absolutely nothing. What scanner? What issues? I strongly suspect this should be 3 separate commits, not one bundled up.

The issues was found by Red Hat's OpenScanHub:

    fio-3.35/engines/rdma.c:279:3: warning[deadcode.DeadStores]:
            Value stored to 'ret' is never read

Signed-off-by: Pavel Reichl <preichl@redhat.com>
The issue was found by Red Hat's OpenScanHub:

    fio-3.35/server.c:1884:3: warning[deadcode.DeadStores]:
            Value stored to 'extended_buf_wp' is never read

Signed-off-by: Pavel Reichl <preichl@redhat.com>
@preichl
Copy link
Contributor Author

preichl commented Dec 15, 2023

Thank you for comments, please see 2nd version.

io_u.h Outdated
@@ -81,7 +81,7 @@ struct io_u {
struct io_piece *ipo;

unsigned long long resid;
unsigned int error;
int error;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes like this HAS to be done very carefully, and not just because some static checker has a blurb about it. The error field is positive values, did you consider that perhaps it's that one use case that may be broken? That would be worthwhile fixing up.

@axboe
Copy link
Owner

axboe commented Dec 15, 2023

That io_u->error change is highly suspect imho. The other 3 patches look fine. I'd suggest you drop that one for now and we can get the other 3 applied, and then take a deeper look at if this is an inconsistent use of the error field in http instead.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
@axboe
Copy link
Owner

axboe commented Dec 15, 2023

Pulled with that dropped, and then fixed up the error in http.

@axboe axboe merged commit 5a13bb8 into axboe:master Dec 15, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants