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

msg/async/rdma: fix assign instead of compare #16664

Merged
merged 1 commit into from Aug 11, 2017

Conversation

Projects
None yet
3 participants
@amitkumar50
Copy link
Contributor

commented Jul 28, 2017

Fixed:

** CID 1414508 (#1 of 1): Assign instead of compare (PW.ASSIGN_WHERE_COMPARE_MEANT)

  1. assign_where_compare_meant: use of "=" where "==" may have been intended

Signed-off-by: Amit Kumar amitkuma@redhat.com

@joscollin joscollin added the cleanup label Jul 29, 2017

@joscollin joscollin self-requested a review Jul 29, 2017

@@ -603,7 +603,7 @@ void RDMAConnectedSocketImpl::notify()
int ret;

ret = write(notify_fd, &i, sizeof(i));
assert(ret = sizeof(i));
assert(ret == sizeof(i));

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

Nit:
1, Why doing sizeof(i) two times ?
2. Conventionally the expected value is the first argument.

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member
  1. You can finish it off in a single statement too.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 Jul 29, 2017

Author Contributor

@joscollin Thanks for comments. since function returns void no point in taking ret also.
Can we make this way?
assert(sizeof(i) == write(notify_fd, &i, sizeof(i)));

write() Returns the number of bytes that were written. If value is negative, then the system call returned an error.
assert(exp) if expression evaluates to FALSE, displays an error message aborts.

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

@amitkumar50
Yes that's what I meant in my comment 2 and 3. What about 1 ?

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 Jul 29, 2017

Author Contributor

@joscollin Yes we can replace whole function as something:
void RDMAConnectedSocketImpl::notify()
{
int i = 1;
assert(sizeof(notify_fd) == write(notify_fd, &i, sizeof(i)));
}

Changed type of i, because notify_fd is
msg/async/rdma/RDMAStack.h
int notify_fd = -1;

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 Jul 29, 2017

Author Contributor

@joscollin And I believe sizeof() check has to be there.
Since sizeof() returns sizeof of type which can be platform dependent.
We cannot compare return value of write() with an value.

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

@amitkumar50
The return value of write() should be compared with the bytes written. So comparing with sizeof(i) makes sense to me. Another point is this change may lead to a signed-unsigned comparison warning too, if not type casted appropriately.

This comment has been minimized.

Copy link
@amitkumar50

amitkumar50 Jul 29, 2017

Author Contributor

@joscollin
write() returns number of bytes that were written That means Its not TYPE, its value.
sizeof() returns size(in bytes) of its operand or type passed. Not type itself.

So I suppose this comparison is type-safe as both Left, Right hand returns ints not types
assert(sizeof(notify_fd) == write(notify_fd, &i, sizeof(i)));

@joscollin joscollin added bug fix core and removed cleanup labels Jul 29, 2017

@@ -603,7 +603,7 @@ void RDMAConnectedSocketImpl::notify()
int ret;

ret = write(notify_fd, &i, sizeof(i));
assert(ret = sizeof(i));
assert(ret == sizeof(i));

This comment has been minimized.

Copy link
@joscollin

joscollin Jul 29, 2017

Member

@amitkumar50
The return value of write() should be compared with the bytes written. So comparing with sizeof(i) makes sense to me. Another point is this change may lead to a signed-unsigned comparison warning too, if not type casted appropriately.

@amitkumar50 amitkumar50 force-pushed the amitkumar50:coverity-1414508-3 branch from e7df0b5 to a891962 Jul 29, 2017

@joscollin

This comment has been minimized.

Copy link
Member

commented Jul 30, 2017

@amitkumar50 Rebase and squash commits.

@amitkumar50 amitkumar50 force-pushed the amitkumar50:coverity-1414508-3 branch from 092e3f3 to 2e75b87 Jul 30, 2017

@amitkumar50

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2017

@joscollin Done Thanks..

rdma: Assign instead of compare
Fixed:

** CID 1414508 (#1 of 1): Assign instead of compare (PW.ASSIGN_WHERE_COMPARE_MEANT)
1. assign_where_compare_meant: use of "=" where "==" may have been intended

Signed-off-by: Amit Kumar amitkuma@redhat.com
@joscollin

This comment has been minimized.

Copy link
Member

commented Aug 10, 2017

Jenkins retest this please

@yuyuyu101 yuyuyu101 merged commit 2cf5a75 into ceph:master Aug 11, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@yuyuyu101 yuyuyu101 changed the title rdma: Assign instead of compare msg/async/rdma: fix assign instead of compare Aug 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.