<https://forums.FreeBSD.org/threads/82247/post-533935> expresses a requirement for procfs that is not yet expressed in the package messages for wine and wine-devel. <https://www.freshports.org/emulators/wine/#message> <https://cgit.freebsd.org/ports/tree/emulators/wine/files/pkg-message.in> <https://www.freshports.org/emulators/wine-devel/#message> <https://cgit.freebsd.org/ports/tree/emulators/wine-devel/files/pkg-message.in> ---- Compare with the expressions at: <https://www.freshports.org/emulators/i386-wine/#message> <https://www.freshports.org/emulators/i386-wine-devel/#message>
Created attachment 228786 [details] Proposed patch I am planning to apply this patch to add documentation. Alex, Damjan, any thoughts? (And, Damjan, also fine for wine-devel then?)
(In reply to Gerald Pfeifer from comment #1) Everything in FreeBSD procfs has either sysctl or libprocstat equivalent: https://forums.freebsd.org/threads/i-cant-run-windows-applications-with-wine-64bit.77253/#post-522279.
(In reply to Alex S from comment #2) It would be better to use sysctl in the long run. I am currently looking at patching Wine to do that.
Patches to use sysctl() calls instead of /proc/curproc/file everywhere in Wine are now available, and get Wine working with /proc unmounted: https://source.winehq.org/patches/data/218031 https://source.winehq.org/patches/data/218032 https://source.winehq.org/patches/data/218033 https://source.winehq.org/patches/data/218034 https://source.winehq.org/patches/data/218035 https://source.winehq.org/patches/data/218036 https://source.winehq.org/patches/data/218037 https://source.winehq.org/patches/data/218038 https://source.winehq.org/patches/data/218039 I'll try include this, along with the many patches needed to get the new version to build :(.
(In reply to Damjan Jovanovic from comment #4) So cool.
(In reply to Damjan Jovanovic from comment #4) The patches themselves are great, but now I can't unsee those __FreeBSD_kernel__ ifdefs lying around :/ Looks like GNU/kFreeBSD stuff [1]. What a bunch of assholes... On a more serious note, there are places where Wine tries to access /proc/self/pagemap, /proc/uptime, /proc/stat, /proc/%u/mem, /proc/self/fd/%u. That code would be very well worth reviewing (as time permits, of course). [1]: https://lists.freebsd.org/pipermail/freebsd-hackers/2011-July/035721.html
(In reply to Alex S from comment #6) Damn, you're right. Most /proc usage is #ifdef linux, but I somehow missed a few places that aren't. What's wrong with GNU/kFreeBSD? Isn't more FreeBSD usage better?
(In reply to Damjan Jovanovic from comment #7) > What's wrong with GNU/kFreeBSD? Isn't more FreeBSD usage better? You might notice the macro name is very generic (which is a deliberate choice vs something like __Debian_GNU_kFreeBSD__ according to the aformentioned mailing list discussion), while the ifdefed parts actually reference a Linux-specific procfs path. So much for interoperability. Anyway, GNU/kFreeBSD is thankfully long gone, so this crap is safe to remove.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=a269704700c2265a3c17b879aa5922ccf12c1c1e commit a269704700c2265a3c17b879aa5922ccf12c1c1e Author: Gerald Pfeifer <gerald@FreeBSD.org> AuthorDate: 2021-10-29 06:05:31 +0000 Commit: Gerald Pfeifer <gerald@FreeBSD.org> CommitDate: 2021-10-29 06:05:31 +0000 emulators/wine: Document the need for /proc to be mounted This likely won't be necessary with the next major version any longer due to upstream work by Damjan; for the time being it is still required. PR: 258795 emulators/wine/Makefile | 2 +- emulators/wine/files/pkg-message.in | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-)
This should address the issue for emulators/wine via documentation. Thanks to Damjan upstream Wine development has many addressed already, which the next to next version of emulators/wine-devel will get automatically. @Damjan, given there's some more which you haven't go upstream yet, maybe go for 6.20 without any of those patches, just the documentation part?
(In reply to Gerald Pfeifer from comment #10) Sure, release the 6.20 port without my patches.
An overview of the remaining /proc work: dlls/ntdll/unix/system.c: if ((fp = fopen("/proc/uptime", "r"))) This seems to be used for system idle time since boot. Ours is the CP_IDLE (5th) field in "sysctl kern.cp_time". It is set by statclock() in sys/kern/kern_clock.c, which runs every statperiod in sys/kern/kern_clocksource.c where statperiod = SBT_1S / stathz; and stathz can be queried using "sysctl kern.clockrate" which gives: kern.clockrate: { hz = 1000, tick = 1000, profhz = 8128, stathz = 127 } ie. it runs 127 times per second. However, the fallback code for when /proc/uptime can't be opened, which just increments a sequential counter, says: /* many programs expect IdleTime to change so fake change */ and 127 times per second won't change often, so we are probably better off using that fallback code. dlls/ntdll/unix/system.c: FILE *cpuinfo = fopen("/proc/stat", "r"); Per CPU time statistics, our "sysctl kern.cp_times". We should implement this. dlls/ntdll/unix/virtual.c: f = fopen( "/proc/self/pagemap", "rb" ); Needed, probably our /proc/curproc/map or equivalent libprocstat call. dlls/ntdll/unix/file.c: if ((f = fopen( "/proc/mounts", "r" ))) Not a /proc issue, but only Linux and Apple are supported when querying mount points, the Apple code should be reused for FreeBSD. server/process.c: sprintf( proc_path, "/proc/%u/status", process->unix_pid ); #ifdef linux, but nothing else is supported. server/ptrace.c: sprintf( procmem, "/proc/%u/mem", process->unix_pid ); server/ptrace.c: sprintf( procmem, "/proc/%u/mem", process->unix_pid ); Optimization for reading/writing large amounts of memory instead of calling ptrace() many times. Needs to be made Linux-only. server/change.c: The code takes all sorts of liberties when HAVE_SYS_INOTIFY_H is defined and uses /proc/self/fd which we never had.
Thanks, folks 👍
(In reply to Damjan Jovanovic from comment #12) A: dlls/ntdll/unix/system.c: if ((fp = fopen("/proc/uptime", "r"))) https://source.winehq.org/patches/data/218440 I implemented this anyway, as I realized Linux's /proc/uptime also has low resolution, and Linux is heavily used, so that comment: /* many programs expect IdleTime to change so fake change */ must be wrong. B: dlls/ntdll/unix/system.c: FILE *cpuinfo = fopen("/proc/stat", "r"); https://source.winehq.org/patches/data/218439 Implemented. C: dlls/ntdll/unix/virtual.c: f = fopen( "/proc/self/pagemap", "rb" ); https://source.winehq.org/patches/data/218444 Implemented, but not very well. FreeBSD doesn't seem to offer memory page status (eg. shared, swapped out) at a page level, only at a page range level. D: dlls/ntdll/unix/file.c: if ((f = fopen( "/proc/mounts", "r" ))) https://source.winehq.org/patches/data/218441 This was easy, we just reuse the code for Apple. E: server/process.c: sprintf( proc_path, "/proc/%u/status", process->unix_pid ); https://source.winehq.org/patches/data/218442 Mostly good. FreeBSD doesn't seem to offer maximum virtual memory usage, only maximum RSS usage. F: dlls/ntdll/unix/process.c: strcpy( path, "/proc/self/status" ); dlls/ntdll/unix/process.c: sprintf( path, "/proc/%u/status", unix_pid); https://source.winehq.org/patches/data/218443 Same as (E). G: server/ptrace.c: sprintf( procmem, "/proc/%u/mem", process->unix_pid ); server/ptrace.c: sprintf( procmem, "/proc/%u/mem", process->unix_pid ); That /proc/%u/mem is only used for better performance; when it can't be opened, the code already falls back to ptrace. It should work on FreeBSD too, when /proc is mounted. I couldn't find any other way to efficiently access the memory of another process, so I just left it. H: server/change.c: (4 places) https://source.winehq.org/patches/data/218445 The crowning jewel of this patch set. The inotify file change notifications use Linux's /proc/self/fd/%d symlink to translate a directory's fd to its filesystem path. Even using libprocstat for this, would work poorly on FreeBSD, as our fd->path cache is lossy. Luckily, Wine already stores the filesystem paths internally, and patching the code to use them instead of /proc, gets the dlls/kernel32/tests/change.c unit tests from badly failing to fully passing :-).
(In reply to Damjan Jovanovic from comment #11) > Sure, release the 6.20 port without my patches. Apologies, I failed to Cc: me on this bug somehow and missed your response, so applied the update including the initial set of packages. Doesn't hurt, they work and we'll remove them when the 6.21 update lands. And ... great work on all those upstream patches, Damjan. Huge thumbs up!
(In reply to Gerald Pfeifer from comment #15) > ...packages... ...patches...
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/ports/commit/?id=00078ad233b83f1b5a53355502ba8747480c991f commit 00078ad233b83f1b5a53355502ba8747480c991f Author: Damjan Jovanovic <damjan.jov@gmail.com> AuthorDate: 2021-11-07 08:31:42 +0000 Commit: Gerald Pfeifer <gerald@FreeBSD.org> CommitDate: 2021-11-07 08:31:42 +0000 emulators/wine-devel: Update to Wine 6.21 This includes the following changes: - WinSpool, GPhoto, and a few other modules converted to PE. - Better support for inline functions in DbgHelp. - Beginnings of a MSDASQL implementation. - Various bug fixes. files/patch-dlls-winebus-6.20 and files/patch-no-procfs have been accepted upstream, so remove our local copies. With that, and additional upstream changes, mounting /proc is no longer required on FreeBSD, so we don't need to document it. [1] PR: 258795 [1] Approved by: maintainer (= author) emulators/wine-devel/Makefile | 3 +- emulators/wine-devel/distinfo | 10 +- .../files/patch-dlls-winebus-6.20 (gone) | 36 --- emulators/wine-devel/files/patch-no-procfs (gone) | 284 --------------------- emulators/wine-devel/pkg-plist | 16 +- 5 files changed, 14 insertions(+), 335 deletions(-)
Not all my patches were accepted yet. In particular, the VM counters one still tries to use /proc: https://source.winehq.org/patches/data/218442
I was still going to close this PR since emulators/wine has been addressed (via documentation), most of emulators/wine-devel has been addressed (via code) and the rest is in flow/submitted, so documenting this for wine-devel now doesn't seem like the way to go. What do you think?
(In reply to Gerald Pfeifer from comment #19) > … What do you think? I'm ambivalent about closure. More than happy with what's done, and progressing.
The last procfs patch was that inotify file change notifications patch, which was rejected due to the fact we can't use Wine's internal filenames, as files can be renamed while open but their file descriptors remain the same. I've made and submitted another patch (https://source.winehq.org/patches/data/221281) which queries the fd's path using the KERN_PROC_FILEDESC sysctl, and fixed a bug in my original patch (r was 0 instead of strlen(name), so the wrong path was being returned). However with that bug fixed, both my original patch and the new one fail a particular test case, which seems to be some kind of bug in our libinotify. This is a separate issue though.
Cool! Note, don't break the build on non-Linux, non-FreeBSD platforms with an #error, just return a dummy value. (This did not work there before, and you are adding FreeBSD support, not removing anything, right?) Make it a run-time message such as ERR("... not implemented for this platform."); instead.
Closing this, after an exchange between Damjan and myself, as the procps requirement as such is gone. There is one patch upstream still needs to take around inotify, alas that's separate.