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

NFS - Fixing Coverity issue (Dereference null return value) #1894

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

BarakSason
Copy link
Contributor

@BarakSason BarakSason commented Dec 7, 2020

CID 1430123
Fixing dereference null return value by checking the value returned by
an allocating method

Change-Id: I3fc18208fd4cec2db4b2b5d1f47ef7d7f1c9a4b3
updates: #1060
Signed-off-by: Barak Sason Rofman bsasonro@redhat.com

CID 1430123
Fixing dereference null return value by checking the value returned by
an allocating method

Change-Id: I3fc18208fd4cec2db4b2b5d1f47ef7d7f1c9a4b3
updates: gluster#1060
Signed-off-by: Barak Sason Rofman <bsasonro@redhat.com>
@@ -1424,8 +1424,10 @@ nlm4svc_lock_cbk(call_frame_t *frame, void *cookie, xlator_t *this,
ncf = nlm4_notify_init(cs);
if (ncf) {
ncf->frame = copy_frame(frame);
ncf->frame->local = ncf;
nlm4svc_send_granted(ncf);
if(ncf->frame){
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue that if copy_frame() failed, something is really really bad - out of memory for example, and you need to bail out.
This does fix the coverity issue perhaps, but not the real issue.

Copy link
Contributor Author

@BarakSason BarakSason Dec 7, 2020

Choose a reason for hiding this comment

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

While fixing this issue I've looked how this kind of situation is handled throughout the code.
In most cases code the code is in the form of:

var = copy_frame(frame);
    if (var == NULL) {
        goto out;
}

Moreover, the method calling this one is ignoring the return value anyway (this probably needs to be fixed, but not in the scope of the Coverity fix as this fix will involve functional change)

@BarakSason
Copy link
Contributor Author

/recheck smoke

@BarakSason
Copy link
Contributor Author

/run regression

1 similar comment
@BarakSason
Copy link
Contributor Author

/run regression

@BarakSason BarakSason merged commit 085790d into gluster:devel Dec 17, 2020
@BarakSason BarakSason deleted the CID1430123 branch December 17, 2020 09:30
@itisravi
Copy link
Member

I think the upstream tests are not catching clang-format errors now. I get this (tabs instead of spaces):
image

@BarakSason
Copy link
Contributor Author

I think the upstream tests are not catching clang-format errors now. I get this (tabs instead of spaces):
image

@deepshikhaaa Can you please check this?
@itisravi Should I revert this?

@itisravi
Copy link
Member

Should I revert this?

You could just send another patch when you get to it.

@amarts
Copy link
Member

amarts commented Dec 18, 2020

@itisravi this issue of clang-format not picking the proper changes is now fixed. gluster/build-jobs#63

I recommend not reverting, but to fix the particular file in another PR.

@itisravi
Copy link
Member

@amarts Thanks for the fix! Yes another PR was what I was suggesting as well.

BarakSason added a commit to BarakSason/glusterfs that referenced this pull request Dec 20, 2020
Smoke tests verified PR gluster#1894
even though there was a clang issue present and the PR was merged.
Smoke tests have been fixed so sending this PR to rectify the clang
issue.

Change-Id: I3df5d2c77d9f3dd1872f2f28824565d5f24d82ec
updates: gluster#1060
Signed-off-by: Barak Sason Rofman <bsasonro@redhat.com>
itisravi pushed a commit that referenced this pull request Dec 21, 2020
Smoke tests verified PR #1894
even though there was a clang issue present and the PR was merged.
Smoke tests have been fixed so sending this PR to rectify the clang
issue.

Change-Id: I3df5d2c77d9f3dd1872f2f28824565d5f24d82ec
updates: #1060
Signed-off-by: Barak Sason Rofman <bsasonro@redhat.com>
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.

None yet

4 participants