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

hp-ux port #409

Closed
wants to merge 2 commits into from
Closed

hp-ux port #409

wants to merge 2 commits into from

Conversation

malexzx
Copy link
Contributor

@malexzx malexzx commented Dec 1, 2016

Hello!

Following patch allows the mongo-c driver to work on HP-UX 11.31 ia64 platform.
Note: to work, with this patch, we need a patch to libbson, that PR-ed just now (mongodb/libbson#179).

To compile on HPUX we need:

  • gcc-4.8.5
  • pass -D_REENTRANT -D_INCLUDE_HPUX_SOURCE to CFLAGS
  • pass -mlp64 to CFLAGS and LDFLAGS for 64bit version

configure opts: --enable-shm-counters=no -sinse it not ported yet

Copy link
Contributor

@bjori bjori left a comment

Choose a reason for hiding this comment

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

thank you so much! it looks great!
I've made few inline very nitpicky comments.
Let me know if you can update this request, otherwise we'll gladly do so and amend the commit


#ifdef MAX
#undef MAX
#endif
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel this is a bit unfortunate. we should probably rename the macros below instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @bjori
I can update this PR.

I will try to rename, but it comes from HP system header, and probably no luck with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code below this macro - define that macro as
#define MAX "max" - so gcc warn about macro redefinition.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, yeah. I mean, we should rename our MAX macro there to something else.

MAX and MIN are quite common macros which we shouldn't be imposing on.
we should probably rename our macros to MAX_STRING, and MAX_STRING_LEN maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that should be reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -38,6 +38,12 @@
# include <sys/un.h>
#endif

#ifdef __hpux__
typedef int mongoc_socklen_t;
Copy link
Contributor

@bjori bjori Dec 1, 2016

Choose a reason for hiding this comment

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

we should probably move this to autoconf check, I can imagine more unixes not having socklen_t

Copy link
Contributor Author

Choose a reason for hiding this comment

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

may be that right.
I not familiar with autoconf, may be you do that in right way (to define HAVE_XXX),

Copy link
Contributor

Choose a reason for hiding this comment

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

will do ! leave it as is then, its fine :)

@@ -487,7 +487,7 @@ mongoc_socket_accept_ex (mongoc_socket_t *sock, /* IN */
int
mongoc_socket_bind (mongoc_socket_t *sock, /* IN */
const struct sockaddr *addr, /* IN */
socklen_t addrlen) /* IN */
mongoc_socklen_t addrlen) /* IN */
Copy link
Contributor

Choose a reason for hiding this comment

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

here, and everywhere else, the argument should be aligned in a pretty ascii art with the argument above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i will do allign it now.

@@ -48,6 +51,12 @@ _mongoc_get_cpu_count (void)
{
#if defined(__linux__)
return get_nprocs ();
#elif defined(__hpux__)
struct pst_dynamic psd;
if (pstat_getdynamic (&psd, sizeof (psd), (size_t) 1, 0) != -1)
Copy link
Contributor

Choose a reason for hiding this comment

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

our coding standard requires braces, even for onelines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, undestand

@@ -333,7 +333,7 @@ _mongoc_socket_capture_errno (mongoc_socket_t *sock) /* IN */
#else
sock->errno_ = errno;
#endif
TRACE("setting errno: %d", sock->errno_);
TRACE("setting errno: %d %s", sock->errno_, strerror(sock->errno_));
Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense :D

@malexzx
Copy link
Contributor Author

malexzx commented Dec 1, 2016

now amend this commit with update allignment of variables and add braces to conform coding standart.
if I have missed something, please tell me about that.

@malexzx
Copy link
Contributor Author

malexzx commented Dec 1, 2016

note, this PR requires libbson PR

@bjori
Copy link
Contributor

bjori commented Dec 9, 2016

Sorry to leave you in the dark on this.
We are working on getting consistent access to HPUX so we can ensure it will continue to be supported platform.

@malexzx
Copy link
Contributor Author

malexzx commented Dec 10, 2016

HPUX 11.31 (11i v3)
Integrity (aka ia64) is the latest hw platform supported by HPUX.
Sure, it supported https://h20392.www2.hpe.com/portal/swdepot/displayProductsList.do?category=OE

to bootstrap gcc-4.8.5 on HPUX use this link https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64919

@bjori
Copy link
Contributor

bjori commented Dec 29, 2016

I've created https://jira.mongodb.org/browse/CDRIVER-1973 so we can keep track of this patch when we prepare for our next release.
I find it unlikely we would include this in 1.5.2, but we plan on shipping 1.6.0 fairly soon

@malexzx
Copy link
Contributor Author

malexzx commented Dec 30, 2016

cool news! I see some conflicts in this branch. But now I'm at vacation now (to 12 January) , so not possible to help to resolve it. But as most of it - is a indentation and type replacement. That is not hard to change.
Anyway, very glad to that fact, so this patch is ready for including in release!

@bjori
Copy link
Contributor

bjori commented Dec 30, 2016

Don't worry about the conflict, they are caused by our coding standard consistency commit in master. I'll fix it up when I merge this

@bjori
Copy link
Contributor

bjori commented Jan 24, 2017

Merged as

Thanks for the patch!

@bjori bjori closed this Jan 24, 2017
@malexzx
Copy link
Contributor Author

malexzx commented Feb 2, 2017

Hello @bjori !
Thank you for support!

Everithing works, tests pass, but small fixes need:

  1. The method for determine of using socklen_t does not work. socklen_t present on HPUX. It needs different socket implementation. I suggest to check use int by test macro _XOPEN_SOURCE_EXTENDED not set. For example, the ACE library check it with this code:
#    if defined (_XOPEN_SOURCE_EXTENDED)
typedef socklen_t ACE_SOCKET_LEN;
#    else
typedef int ACE_SOCKET_LEN;
#    endif /* _XOPEN_SOURCE_EXTENDED */
#  else
typedef socklen_t ACE_SOCKET_LEN;
#  endif /* __hpux */
#elif defined (ACE_HAS_SIZET_SOCKET_LEN)
typedef size_t ACE_SOCKET_LEN;
#else
typedef int ACE_SOCKET_LEN;
#endif /* ACE_HAS_SIZET_SOCKET_LEN */

(see http://nixdoc.net/man-pages/HP-UX/man2/connect.2.html for more information)
Lack of this macro will select native BSD socket implementation (instead of emulating GNU style sockets, which has a bugs). I always use BSD variant.
2) getting versions from VERSION_* files with HPUX utils fails, if those files not contain \n at end (I will investigate to avoid this).

@bjori
Copy link
Contributor

bjori commented Feb 2, 2017

The above code will blindly ignore socklen_t and always pass int as the 3rd argument to connect, rather then size_t in the weird case where HPUX actually expects size_t.
If the type is however available, we use the native type, which will match the type expected by connect. I don't understand what you mean that how we typedef mongoc_socklen_t will affect which socket implementation will be used by HPUX?

I don't really think this matters much as the value of addrlen is never going to be so large for this to matter. Matching the strict prototype though would avoid a possible compiler warning.

As for the VERSION files -- I'm very curious, I couldn't see any such issues when I was building on HPUX the other day. Could you file a ticket for the issue at https://jira.mongodb.org/browse/CDRIVER ?

@malexzx
Copy link
Contributor Author

malexzx commented Feb 2, 2017

HPUX have several implementation of sockets. With case of connect() call - conversion things happens
, but passing pointers to receive (like in getsockname(... , int *)) will produce issue (on big endian platform). (I file the bug https://jira.mongodb.org/browse/CDRIVER-2015)
You can try simple test to view issue:
test.c

#include <stdio.h>
#include <sys/socket.h>

void test(int *i, socklen_t *s){
  printf("i=%d s=%d\n", *i, *s);
}

int main(int ac, char *av[]){
 socklen_t a = 1;
 unsigned int i = 10;


 test((int *)&i, (socklen_t*)&i);
 test((int *)&a, (socklen_t*)&a);

 return 0;
}

Output:
BIG_ENDIAN: only two combinations work
i=10 s=314336
i=0 s=1

LITTLE_ENDIAN: portable in any direction
i=10 s=10
i=1 s=1

More detail in https://curl.haxx.se/mail/lib-2009-04/0287.html

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