Bug 192842
| Summary: | [patch][linux] linux_ppoll syscall required by linux apps that use pulseaudio (on linux_base-f20) | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Vassilis Laganakos <vassilis> | ||||||
| Component: | kern | Assignee: | Dmitry Chagin <dchagin> | ||||||
| Status: | Closed FIXED | ||||||||
| Severity: | Affects Only Me | CC: | dchagin, emaste, jilles, mjg, rm, swills, trasz | ||||||
| Priority: | --- | ||||||||
| Version: | CURRENT | ||||||||
| Hardware: | amd64 | ||||||||
| OS: | Any | ||||||||
| Attachments: |
|
||||||||
Assign to maintainer. Some comments about the code (I have not tested this in any way): * Look at pselect() for how to handle the signal mask. The idea of the signal mask parameter is to unblock some signals atomically with starting to wait for events on file descriptors. I think the way you are doing it, an unblocked signal will interrupt ppoll(), but will not call the signal handler. * The variable pap is not needed, you can use &pa where it is used. * Per style(9), the variable pa should be declared at the top of the function. * The less accurate timeout (ms) is probably acceptable for an emulation call like this, but it should be fixed if and when a native ppoll() is added. An overflow check may be appropriate, though: the multiplication tvp->tv_sec * 1000 may overflow and so may the addition to the sub-second part. The special case for 0 seconds is not needed. Created attachment 146818 [details]
linux_ppoll syscall patch for linuxulator v2
Thank you for taking a look in this.
You're right about the pointer to pa variable; I've dropped it and move the declaration of pa early in the function.
Switched back to using ast() as in linux_pselect, dropped the 0s case, and added handling the multiplication and addition potential overflows in a naive way: if the result is smaller than the multiplicator or then the addend, then timeout will get the INT_MAX value.
What's the status of this? Hello. I don't know if there is anything else that I should do, so I'm just wating to see what happens or if anyone asks for something :) The basic testing has been done on the lemul branch. What are the next steps for this to go in tree? +linux_ppoll(struct thread *td, struct linux_ppoll_args *pargs)
+{
+ struct l_timespec lts;
+ struct timeval tv, *tvp, tv0, tv1;
+ struct poll_args pa;
+ sigset_t usigmask, *usigmaskp;
+ l_sigset_t lsigmask;
+ int error, retval, mss, msns;
error and retval?
+
+ /* Get the time */
+ if (pargs->timeout_ts != NULL)
+ {
style
+ error = copyin(pargs->timeout_ts, <s, sizeof(lts));
+ if (error != 0)
+ return (error);
+
+ TIMESPEC_TO_TIMEVAL(&tv, <s);
+ if (itimerfix(&tv))
+ return (EINVAL);
+
+ /* Mark the time before the call */
+ microtime(&tv0);
+ tvp = &tv;
+ } else
+ tvp = NULL;
+
+ /* Get the sigmask */
+ if (pargs->sigmask != NULL) {
+ error = copyin(pargs->sigmask, &lsigmask, sizeof(lsigmask));
+ if (error)
+ return (error);
+ linux_to_bsd_sigset(&lsigmask, &usigmask);
+ usigmaskp = &usigmask;
+ } else
+ usigmaskp = NULL;
+
+ /* Set the sigmask */
+ error = kern_sigprocmask (td, SIG_SETMASK, usigmaskp, &td->td_oldsigmask, 0);
+ if (error)
+ return (error);
+
+ pa.fds = pargs->fds;
+ pa.nfds = pargs->nfds;
+
+ /* Linux's ppoll allows NULL timeout which is translated to FreeBSD's INFTIM (-1) */
+ if (tvp==NULL)
+ pa.timeout = INFTIM;
+ else
+ {
style
+ mss = tvp->tv_sec * 1000;
+ msns = tvp->tv_usec / 1000;
+
+ /*
+ * Handling the multiplication and addition overflow.
+ * If it happens, assing pa.timeout the INT_MAX value
+ */
+ if (mss/1000 == tvp->tv_sec) {
+ pa.timeout = mss + msns;
+ if ( pa.timeout < 0)
+ pa.timeout = INT_MAX;
+ } else
+ pa.timeout = INT_MAX;
+ }
+
+ /*
+ * Ensure that the td_oldsigmask is restored by ast()
+ * when returns to user mode and that the TDP_OLDMASK
+ * is cleared
+ */
+ td->td_pflags |= TDP_OLDMASK;
+ thread_lock(td);
+ td->td_flags |= TDF_ASTPENDING;
+ thread_unlock(td);
Why was not this done immediately after kern_sigprocmask?
Also, this would benefit from kern_ppoll helper.
Given that pselect has similar code, maybe an additional helper should be introduced.
+
+ /* call sys_poll */
+ retval = sys_poll(td, &pa);
+
+ if (retval == 0 && pargs->timeout_ts) {
+ if (td->td_retval[0]) {
+ /*
+ * Compute how much time was left of the timeout,
+ * by subtracting the current time and the time
+ * before we started the call, and subtracting
+ * that result from the user-supplied value.
+ */
+ microtime(&tv1);
+ timevalsub(&tv1, &tv0);
+ timevalsub(&tv, &tv1);
+ if (tv.tv_sec < 0)
+ timevalclear(&tv);
+ } else
+ timevalclear(&tv);
+
+ TIMEVAL_TO_TIMESPEC(&tv, <s);
+
+ error = copyout(<s, pargs->timeout_ts, sizeof(lts));
+ if (error)
+ return (error);
+ }
+ return retval;
+}
+
+int
+1 for kern_ppoll(). It would also be great if it was available for native FreeBSD applications. Further look reveals both select and poll require similar code related to timers, so this can be moved into a helper as well. Also these patches have numerous formatting issues not mentioned earlier in my previous comment. What was used to test correctness? At least ltp's tests should be used, and maybe ones from glibc (if they have some for ppoll that is). Not everything has to pass, but getting an idea what's up is definitely helpful Hi,
I tried the second revision of this patch on -current r273290 and it doesn't builds:
cc -O2 -pipe -DCOMPAT_FREEBSD32 -DCOMPAT_LINUX32 -fno-strict-aliasing -Werror -D_KERNEL -DKLD_MODULE -nostdinc -DHAVE_KERNEL_OPTION_HEADERS -include /mnt/media/d1/obj/mnt/media/d1/src/sys/Z1/opt_global.h -I. -I@ -I@/contrib/altq -fno-common -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -I/mnt/media/d1/obj/mnt/media/d1/src/sys/Z1 -mcmodel=kernel -mno-red-zone -mno-mmx -mno-sse -msoft-float -fno-asynchronous-unwind-tables -ffreestanding -fstack-protector -mno-aes -mno-avx -Qunused-arguments -std=iso9899:1999 -fstack-protector -Wall -Wredundant-decls -Wnested-externs -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Winline -Wcast-qual -Wundef -Wno-pointer-sign -fformat-extensions -Wmissing-include-dirs -fdiagnostics-show-option -Wno-error-tautological-compare -Wno-error-empty-body -Wno-error-parentheses-equality -Wno-error-unused-function -mno-aes -mno-avx -Qunused-arguments -c /mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:581:13: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
if (pargs->timeout_ts != NULL)
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:583:25: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
error = copyin(pargs->timeout_ts, <s, sizeof(lts));
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:598:13: error: no member named 'sigmask' in 'struct linux_ppoll_args'
if (pargs->sigmask != NULL) {
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:599:25: error: no member named 'sigmask' in 'struct linux_ppoll_args'
error = copyin(pargs->sigmask, &lsigmask, sizeof(lsigmask));
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:612:18: error: no member named 'fds' in 'struct linux_ppoll_args'
pa.fds = pargs->fds;
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:613:19: error: no member named 'nfds' in 'struct linux_ppoll_args'
pa.nfds = pargs->nfds;
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:648:28: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
if (retval == 0 && pargs->timeout_ts) {
~~~~~ ^
/mnt/media/d1/src/sys/modules/linux/../../compat/linux/linux_misc.c:666:32: error: no member named 'timeout_ts' in 'struct linux_ppoll_args'
error = copyout(<s, pargs->timeout_ts, sizeof(lts));
~~~~~ ^
8 errors generated.
*** Error code 1
https://reviews.freebsd.org/D1104 https://reviews.freebsd.org/D1105 can be applied to the lemul branch grab in stable/10 now |
Created attachment 146055 [details] linux_ppoll syscall patch for linuxulator While testing the linux_base-f20 and rest of -f20 ports, I was trying to get Skype to work with pulseaudio on the native side. For that to work seems that it needs at least 2 more syscalls in the linuxulator, fstatfs64 and ppoll. The latter one is most likely required by any linux app that uses pulse audio. So this is a naive implementation of linux_ppoll, tested to work with some pulseaudio command line utils (e.g. /compat/linux/usr/bin/paplay) playing correctly a few wav files. The machine is amd64 running lemul (which is a couple of weeks behind CURRENT but the patch was ported). I followed the guidelines from man (2) ppoll from Linux. I don't know if there is a more pretty way to do the save and restore the sigmask, so if there is please let me know so I can update this. I have done some basic testing of the sigmask part of linux_ppoll (e.g. blocking SIGHUP whille polling) and that does seem to work, but not something extansive. paplay seems to pass NULL in its ppoll call, so it wasn't useful for testing this part.