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 broken compilation #13603

Merged
merged 1 commit into from Feb 27, 2017
Merged

Conversation

Adirl
Copy link

@Adirl Adirl commented Feb 23, 2017

following api change in commit eb0f624

Signed-off-by: Sarit Zubakov saritz@mellanox.com

Resposible commit is: eb0f624

issue:985021

Change-Id: I5b4afc537a9351dd18f5ea08bf4d72a1b03a8635
Signed-off-by: Sarit Zubakov <saritz@mellanox.com>
@Adirl
Copy link
Author

Adirl commented Feb 23, 2017

@yuyuyu101

@@ -97,7 +97,7 @@ int RDMAServerSocketImpl::accept(ConnectedSocket *sock, const SocketOptions &opt
::close(sd);
return -errno;
}
net.set_priority(sd, opt.priority);
net.set_priority(sd, opt.priority, out->get_family());
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to set priority after assigning value to "out"

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yuyuyu101 ,
How about the following (change order put set_sockadde() before)?

`int RDMAServerSocketImpl::accept(ConnectedSocket *sock, const SocketOptions &opt, entity_addr_t *out, Worker *w)
{
ldout(cct, 15) << func << dendl;

assert(sock);
sockaddr_storage ss;
socklen_t slen = sizeof(ss);
int sd = ::accept(server_setup_socket, (sockaddr*)&ss, &slen);
if (sd < 0) {
return -errno;
}
ldout(cct, 20) << func << " accepted a new QP, tcp_fd: " << sd << dendl;

net.set_close_on_exec(sd);
int r = net.set_nonblock(sd);
if (r < 0) {
::close(sd);
return -errno;
}

r = net.set_socket_options(sd, opt.nodelay, opt.rcbuf_size);
if (r < 0) {
::close(sd);
return -errno;
}

if (out)
out->set_sockaddr((sockaddr*)&ss);
net.set_priority(sd, opt.priority, out->get_family());

RDMAConnectedSocketImpl* server;
//Worker* w = dispatcher->get_stack()->get_worker();
server = new RDMAConnectedSocketImpl(cct, infiniband, dispatcher, dynamic_cast<RDMAWorker*>(w));
server->set_accept_fd(sd);
ldout(cct, 20) << func << " accepted a new QP, tcp_fd: " << sd << dendl;
std::unique_ptr csi(server);
*sock = ConnectedSocket(std::move(csi));

return 0;
}
`

Copy link
Member

Choose a reason for hiding this comment

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

yes

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yuyuyu101, @robbat2 ,
In the original commit: "msg/async: support IPv6 QoS. #13418" (#13418), the call to "set_priority()" is done without check if "out" is NULL. We need to first check if out is not NULL before sending it to "set_priority()". What should be default value if "out" is NULL?

For convenience I copied and marked the essential code to see the problem.

In PosixStack.cc::
260 int PosixServerSocketImpl::accept(ConnectedSocket *sock, const SocketOptions &opt, entity_addr_t *out, Worker w) {

281 handler.set_priority(sd, opt.priority, out->get_family());

285 if (out)
286 out->set_sockaddr((sockaddr
)&ss);
287 return 0;
288 }

Thanks

@Adirl @orendu

Copy link
Member

Choose a reason for hiding this comment

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

fire a pr?

Copy link
Contributor

Choose a reason for hiding this comment

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

@saritz i'm trying to see why out should EVER be null inside PosixServerSocketImpl::accept, I'd say we're probably better off with assert(NULL != out)

Copy link
Member

@yuyuyu101 yuyuyu101 left a comment

Choose a reason for hiding this comment

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

see inline

@yuyuyu101 yuyuyu101 merged commit ef19e90 into ceph:master Feb 27, 2017
@Adirl Adirl deleted the fix_compile branch April 18, 2017 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants