Adding sunos.c for solaris/illumos support. #202

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants

I added preliminary solaris/illumos support with event ports in sunos.c as mentioned in #112. The other changes outside sunos.c were related to portability issues between BSD/Linux and SunOS.

This branch was tested with make bench on the following platforms:

  • SmartOS joyent_20130629T040542Z with GCC 4.7.2
  • CentOS+OpenVZ 2.6.32-042stab076.7 with GCC 4.7.1
  • OS X 10.8.4 with clang 4.2

Here's the related documentation for event ports:

Switching to sonos branch worked like a charm on SmartOS, thank you!

Owner

kr commented Apr 3, 2016

This looks good, but I am worried about ongoing maintenance, since I don't have ready access to Solaris/Illumos/SmartOS systems to test. I am happy to merge it anyway, but it'll need some folks to keep an eye on it if changes ever need to happen to the Socket interface.

Do you mind rebasing this? A few of the miscellaneous fixes here have made their way in already.

Contributor

JensRantil commented Apr 3, 2016

In the best of worlds someone could contribute a machine running build+unit tests on Solaris/Illumos/SmartOS so we could get feedback in the pull requests.

I've rebased my changes onto master and checked that the build and unit tests still work correctly on a Joyent public cloud instance:
packager 0d7d688b-50f4-406f-ab94-7654bb3af000 1 2016-04-03 09-27-50
There's probably some way of automating the builds using an ephemeral server on Joyent for each pull request, but I'm not sure whether there's an existing service for that out there or if one would need to be built.

Alternatively, changing all of the socket related operations to the abstractions provided by a third-party event library like libevent or libuv might reduce the complexities of having to manually deal with supporting each of the different event systems at the expense of adding an external dependency.

Owner

kr commented Apr 4, 2016

Alternatively, changing all of the socket related operations to the abstractions provided by a third-party event library like libevent or libuv

Beanstalkd originally used libevent. It was more trouble than it was worth.

I'm not that worried about ongoing maintenance. The other platforms have been pretty hassle-free once the initial bugs are shaken out. Presumably Solaris isn't so bizarre that the same won't hold there as well.

@kr kr and 1 other commented on an outdated diff Apr 4, 2016

@@ -33,13 +33,13 @@ make_conn(int fd, char start_state, tube use, tube watch)
job j;
Conn *c;
- c = new(Conn);
- if (!c) return twarn("OOM"), NULL;
+ c = (Conn *)new(Conn);
@kr

kr Apr 4, 2016

Owner

The changes in this file seem unnecessary. The return value of macro new is void*, which doesn't require a cast to convert to another type. Same goes for testserv.c.

If it does turn out to be necessary, let's do it in a separate pull request.

@potatosalad

potatosalad Apr 4, 2016

This is the error returned without these changes:
beanstalkd beanstalkd-build 1 2016-04-03 21-27-43
This is with GCC 4.7.4 on SmartOS joyent_20160317T000621Z.

@kr

kr Apr 4, 2016

Owner

Out of curiosity, what happens if you run

cc -DNULL='((void*)0)' -Wall -Werror -Wformat=2 -g    -c -o conn.o conn.c

on the original file without these changes?

@potatosalad

potatosalad Apr 4, 2016

It compiles without an error. Same thing for testserv.c. Would adding -DNULL='((void*)0)' to the CFLAGS when sunos is detected be a better solution?

@kr

kr Apr 4, 2016

Owner

I think the thing to do is put something in dat.h like this, with a comment

// Some compilers (e.g. gcc on SmartOS) define NULL as 0.
// This is allowed by the C standard, but is unhelpful when
// using NULL in most pointer contexts with errors turned on.
#ifndef NULL
#define NULL ((void*)0)
#endif

If that still gets the job done cleanly on your system, let's roll with it.

@potatosalad potatosalad commented on an outdated diff Apr 4, 2016

@@ -55,7 +55,7 @@ muststart(char *a0, char *a1, char *a2, char *a3, char *a4)
/* now in child */
- execlp(a0, a0, a1, a2, a3, a4, NULL);
+ execlp(a0, a0, a1, a2, a3, a4, (char*)NULL);
@potatosalad

potatosalad Apr 4, 2016

The error thrown here without this change is:
beanstalkd beanstalkd-build 1 2016-04-03 21-31-38
This is with GCC 4.7.4 on SmartOS joyent_20160317T000621Z.

I tried the #ifndef NULL, but I guess NULL is actually defined as something because I was unable to get it to work. Instead, I added this to dat.h which works as expected:

#if (defined(sun) || defined(__sun)) && (defined(__SVR4) || defined(__svr4__))
#ifdef NULL
#undef NULL
#endif
#define NULL ((void*)0)
#endif

The long #if statement is to distinguish between Solaris and SunOS (which are different operating systems, despite Solaris having the uname of SunOS with SVR4 features added...super confusing).

Also, I just realized my commit message is the exact opposite of what was actually done.

Edit: fixed the commit message.

Is there anything else that needs to be fixed or changed for this PR to be accepted?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment