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

solaris librados port #5909

Closed
wants to merge 2 commits into from
Closed

Conversation

rohanmars
Copy link
Contributor

Signed-off-by: Rohan Mars code@rohanmars.com

@@ -2123,16 +2198,27 @@ int Pipe::do_sendmsg(struct msghdr *msg, int len, bool more)
assert(l == len);
}

int r = ::sendmsg(sd, msg, MSG_NOSIGNAL | (more ? MSG_MORE : 0));
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I guess we could pick up some ways to kick out MSG_NOSIGNAL? @liewegas

@yuyuyu101
Copy link
Member

Maybe async messenger need to impl evport for event methods

-D_PTHREADS \
-D_POSIX_C_SOURCE

endif
Copy link
Contributor

Choose a reason for hiding this comment

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

this may break compilation on OSX and my pending FreeBSD port. please avoid doing this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is -D_REENTRANT required for threading for OSX and FreeBSD? If so I'll add those.

Copy link
Contributor

Choose a reason for hiding this comment

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

guess PTHREAD_CFLAGS offered by ACX_PTHREAD would suffice? and we already have it. and the interesting thing is what i found in m4/acx_pthread.m4 and

        case "${host_cpu}-${host_os}" in
            *-aix* | *-freebsd* | *-darwin*) flag="-D_THREAD_SAFE";;
            *solaris* | *-osf* | *-hpux*) flag="-D_REENTRANT";;
        esac

http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_pthread.m4 shows something different, but similar

 AC_MSG_CHECKING([if more special flags are required for pthreads])
        flag=no
        case ${host_os} in
            aix* | freebsd* | darwin*) flag="-D_THREAD_SAFE";;
            osf* | hpux*) flag="-D_REENTRANT";;
            solaris*)
            if test "$GCC" = "yes"; then
                flag="-D_REENTRANT"
            else
                # TODO: What about Clang on Solaris?
                flag="-mt -D_REENTRANT"
            fi
            ;;
        esac

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets complicated: http://docs.oracle.com/cd/E19455-01/806-5257/6je9h033k/index.html

I chose the pure posix approach. I didn't choose the mixed mode because of compile problems in the boost headers.

Signed-off-by: Rohan Mars <code@rohanmars.com>
@ukernel
Copy link
Contributor

ukernel commented Sep 16, 2015

got following errors on OSX.

msg/simple/Pipe.cc:2249:3: error: use of undeclared identifier 'restore_sigpipe'
restore_sigpipe();
^
msg/simple/Pipe.cc:2615:3: error: use of undeclared identifier 'suppress_sigpipe'
suppress_sigpipe();
^
msg/simple/Pipe.cc:2635:3: error: use of undeclared identifier 'restore_sigpipe'
restore_sigpipe();

I apply your patch on top of my RP #5826

Signed-off-by: Rohan Mars <code@rohanmars.com>
@rohanmars
Copy link
Contributor Author

@ukernel that needed a SO_NOSIGPIPE guard fix. I've just added that to the pull request. thankyou for testing this on OSX!

@liewegas liewegas self-assigned this Oct 6, 2015
// converts from linux errno values to host values
__s32 translate_errno(CephContext *cct,__s32 r)
{
if (r < -34) {
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 a cleaner approach here would be to have a generic

translate_errno()

method that is a #define on Linux but does this conversoin on solaris. Then change all of the return sites in librados.cc to pass the return value through. For example,

https://github.com/ceph/ceph/blob/master/src/librados/librados.cc#L1149

would do

return translate_errno(io_ctx_impl->sparse_read(oid, m, bl, len, off));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liewegas I see pros/cons with both approaches. How will native errors from local systems calls be differentiated using this approach? Using the translation at the unmarshalling layer means that errors from local calls won't be mapped (which is what we want I'm thinking).

Copy link
Member

Choose a reason for hiding this comment

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

On Tue, 27 Oct 2015, rohanmars wrote:

In src/common/solaris_errno.cc:

+#include "messages/MRemoveSnaps.h"
+#include "messages/MRoute.h"
+#include "messages/MStatfs.h"
+#include "messages/MStatfsReply.h"
+#include "messages/MTimeCheck.h"
+#include "messages/MWatchNotify.h"
+
+#define dout_subsys ceph_subsys_ms
+
+#include "solaris_errno.h"
+#include "solaris_message_errno.h"
+
+// converts from linux errno values to host values
+__s32 translate_errno(CephContext *cct,__s32 r)
+{

  • if (r < -34) {

@liewegas I see pros/cons with both approaches. How will native errors from local systems calls be differentiated using
this approach? Using the translation at the unmarshalling layer means that errors from local calls won't be mapped (which
is what we want I'm thinking).

Hmm, yeah. Maybe a better approach would be to create a new set of types:

errorcode32_t
errorcode64_t

that do the translation in the encode/decode methods, and have the magic
conversion methods like

errorcode32_t(int i) : val(i) {}

and

operator int() {
return val;
}

Then we'd just change everything in messages/* that sends a result code
over the wire to use this type instead of int32_t or int64_t...

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I get the approach. I'll try it out. Thanks.

@liewegas
Copy link
Member

liewegas commented Oct 6, 2015

It would help final review if you could break this into a few different patches. One for the errno, one for the socket stuff, one for the sockaddr_storage, etc. Generally looks okay, though! Thanks!

@rohanmars rohanmars closed this Oct 28, 2015
@rohanmars rohanmars deleted the wip-solaris-port branch October 28, 2015 22:06
@rohanmars
Copy link
Contributor Author

Inadvertently closed this pull request.

Created a new one:

#6416

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