Bug 11118 - Add Solaris Ports as a tevent backend
Summary: Add Solaris Ports as a tevent backend
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: 4.2.0rc4
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 10077
  Show dependency treegraph
 
Reported: 2015-02-25 18:55 UTC by Tom Schulz
Modified: 2015-06-15 14:55 UTC (History)
5 users (show)

See Also:


Attachments
Patch to add Solaris Ports as a tevent backend (24.23 KB, text/plain)
2015-02-25 18:55 UTC, Tom Schulz
no flags Details
git-am cherry-pick from master for 4.2.x. (24.04 KB, patch)
2015-02-25 22:38 UTC, Jeremy Allison
ddiss: review+
Details
updated patchset for 4.2 to upgrade to tevent 0.9.23 (34.72 KB, patch)
2015-02-27 16:03 UTC, Michael Adam
obnox: review+
ddiss: review+
Details
Patch for master - 4.2.1 (1.23 KB, patch)
2015-03-03 00:47 UTC, Jeremy Allison
no flags Details
Patches for v4-2-test to include tevent-0.9.24 (9.57 KB, patch)
2015-03-04 09:45 UTC, Stefan Metzmacher
metze: review? (jra)
metze: review? (slow)
obnox: review+
ddiss: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Schulz 2015-02-25 18:55:21 UTC
Created attachment 10789 [details]
Patch to add Solaris Ports as a tevent backend

This patch by Jeremy Allison needs to be included in 4.2.0 release.
Comment 1 Jeremy Allison 2015-02-25 22:38:42 UTC
Created attachment 10792 [details]
git-am cherry-pick from master for 4.2.x.

David, not sure this needs to urgently be in 4.2.0, or we could wait for 4.2.1.
Comment 2 David Disseldorp 2015-02-25 23:25:30 UTC
Comment on attachment 10792 [details]
git-am cherry-pick from master for 4.2.x.

Looks good. It's pretty isolated, so I'm okay with seeing it in 4.2.
Comment 3 Jeremy Allison 2015-02-25 23:27:15 UTC
Re-assigning to Karolin for inclusion in 4.2.0.
Comment 4 Michael Adam 2015-02-27 16:02:00 UTC
Metze asks that we update to the full new version of 0.9.23.
Updated patch following.
Comment 5 Michael Adam 2015-02-27 16:03:23 UTC
Created attachment 10802 [details]
updated patchset for 4.2 to upgrade to tevent 0.9.23
Comment 6 Michael Adam 2015-02-27 16:04:15 UTC
Comment on attachment 10802 [details]
updated patchset for 4.2 to upgrade to tevent 0.9.23

sorry for the hassle,
please re-review.
Comment 7 Michael Adam 2015-02-27 16:17:43 UTC
Thanks David.

Karolin, please pick this updated patchset for 4.2.

Thanks - Michael
Comment 8 Karolin Seeger 2015-03-01 20:25:01 UTC
Pushed to autobuild-v4-2-test.
Comment 9 Tom Schulz 2015-03-02 18:47:01 UTC
I just tried the grand unified patch on Solaris 10 I386 against 4.2.0rc5 and it works the same as the previous versions of the patch.

There is something that I just noticed that I think that I should mention. If I run winbindd with debug level 1, I see the following in the log file:

[2015/02/27 14:21:45.708931,  1] ../lib/util/tevent_debug.c:63(samba_tevent_debug)
  samba_tevent: port_get failed (Interrupted system call)
[2015/02/27 14:21:45.708959,  1] ../source3/winbindd/winbindd_dual.c:1560(fork_domain_child)
  tevent_loop_once failed: Interrupted system call

There is no indication that the above is causing any problem. The oddities that I am seeing when running winbindd are there when the patch is not applied.
Comment 10 David Disseldorp 2015-03-02 18:57:15 UTC
(In reply to Tom Schulz from comment #9)

Thanks a lot for testing this Tom!

482         ret = port_getn(port_ev->port_fd, events, max_events, &nget, &ts);
483         port_errno = errno;
484         tevent_trace_point_callback(ev, TEVENT_TRACE_AFTER_WAIT);
485 
486         if (ret == -1 && port_errno == EINTR && ev->signal_events) {
487                 if (tevent_common_check_signal(ev)) {
488                         return 0;
489                 }
490         }
...
498         if (ret == -1) {
499                 tevent_debug(ev, TEVENT_DEBUG_ERROR,
500                                 "port_get failed (%s)\n",
501                                 strerror(errno));
502                 return -1;
503         }

It looks as though port_getn() is being interrupted by the arrival of a signal, but the event context doesn't have any associated signal event handlers.

@Jeremy / Michael, shouldn't we be dropping back into event handler loop in such a case (ret == -1 && port_errno == EINTR && ev->signal_events == NULL)?
Comment 11 Jeremy Allison 2015-03-02 19:52:02 UTC
I'm always wary of looping inside that tevent call. Looking at the current tevent_epoll.c code, it simply ignores this case and returns 0, so the tevent_poll code should really do the same.

I'll propose a patch for master to do the same for you (ddiss) to review.
Comment 12 Jeremy Allison 2015-03-02 19:52:31 UTC
(In reply to Jeremy Allison from comment #11)

> so the tevent_poll code should really do the same.

I meant tevent_port code of course :-)
Comment 13 Karolin Seeger 2015-03-02 20:02:07 UTC
(In reply to Karolin Seeger from comment #8)
Pushed to v4-2-test.
Re-assigning to Jeremy.
Comment 14 Jeremy Allison 2015-03-03 00:47:10 UTC
Created attachment 10810 [details]
Patch for master - 4.2.1
Comment 15 David Disseldorp 2015-03-03 10:47:00 UTC
(In reply to Jeremy Allison from comment #14)
LGTM, pushed to autobuild.

@Tom: could you please test Jeremy's latest patch?
Comment 16 Tom Schulz 2015-03-03 16:44:47 UTC
The additional tevent patch eliminates the messages in the winbindd log.
Comment 17 David Disseldorp 2015-03-03 23:24:18 UTC
(In reply to Jeremy Allison from comment #14)

Looks like Metze wants a new tevent version tagged and pushed for the 4.2 backport.
Comment 18 Stefan Metzmacher 2015-03-04 09:45:37 UTC
Created attachment 10814 [details]
Patches for v4-2-test to include tevent-0.9.24
Comment 19 David Disseldorp 2015-03-04 09:53:34 UTC
@Karolin, please commit for 4.2.
Comment 20 Karolin Seeger 2015-03-04 10:39:48 UTC
Patch does not apply cleanly to current v4-2-test:

wjende an: Update the tevent_data.dox tutrial stuff to fix some errors, including white space problems.
error: Anwendung des Patches fehlgeschlagen: lib/tevent/doc/tevent_data.dox:46

Please note that an older version has already been pushed.
Please clarify if this needs to be inlcuded in 4.2.0 asap.
Thanks!
Comment 21 David Disseldorp 2015-03-04 11:15:05 UTC
Hi Karolin,

On Wed, 04 Mar 2015 10:39:48 +0000, samba-bugs@samba.org wrote:

> Patch does not apply cleanly to current v4-2-test:
> 
> wjende an: Update the tevent_data.dox tutrial stuff to fix some errors,
> including white space problems.
> error: Anwendung des Patches fehlgeschlagen: lib/tevent/doc/tevent_data.dox:46

The "Update the tevent_data.dox tutrial stuff..." commit is already
present in the v4-2-test branch. You should only need to push Metze's
most recent patch-set - attachment#10814 [details].
Comment 22 Stefan Metzmacher 2015-03-04 14:12:34 UTC
Pushed to v4-2-test