Bug 258795 - emulators/wine emulators/wine-devel review procfs requirements
Summary: emulators/wine emulators/wine-devel review procfs requirements
Status: In Progress
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Many People
Assignee: Damjan Jovanovic
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-09-30 02:26 UTC by Graham Perrin
Modified: 2021-12-03 20:19 UTC (History)
3 users (show)

See Also:


Attachments
Proposed patch (662 bytes, patch)
2021-10-17 20:11 UTC, Gerald Pfeifer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Gerald Pfeifer freebsd_committer 2021-10-17 20:11:17 UTC
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?)
Comment 2 Alex S 2021-10-17 22:32:03 UTC
(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.
Comment 3 Damjan Jovanovic 2021-10-23 04:46:11 UTC
(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.
Comment 4 Damjan Jovanovic 2021-10-23 08:24:37 UTC
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 :(.
Comment 5 Graham Perrin 2021-10-23 17:18:16 UTC
(In reply to Damjan Jovanovic from comment #4)

So cool.
Comment 6 Alex S 2021-10-28 20:41:41 UTC
(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
Comment 7 Damjan Jovanovic 2021-10-29 01:21:47 UTC
(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?
Comment 8 Alex S 2021-10-29 01:51:33 UTC
(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.
Comment 9 commit-hook freebsd_committer 2021-10-29 06:07:52 UTC
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(-)
Comment 10 Gerald Pfeifer freebsd_committer 2021-10-29 06:11:44 UTC
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?
Comment 11 Damjan Jovanovic 2021-10-30 04:53:34 UTC
(In reply to Gerald Pfeifer from comment #10)

Sure, release the 6.20 port without my patches.
Comment 12 Damjan Jovanovic 2021-10-30 06:29:30 UTC
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.
Comment 13 Graham Perrin 2021-10-30 10:02:28 UTC
Thanks, folks 👍
Comment 14 Damjan Jovanovic 2021-11-01 06:07:32 UTC
(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 :-).
Comment 15 Gerald Pfeifer freebsd_committer 2021-11-01 09:22:28 UTC
(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!
Comment 16 Gerald Pfeifer freebsd_committer 2021-11-01 09:45:31 UTC
(In reply to Gerald Pfeifer from comment #15)
> ...packages...

...patches...
Comment 17 commit-hook freebsd_committer 2021-11-07 08:33:01 UTC
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(-)
Comment 18 Damjan Jovanovic 2021-11-07 08:38:53 UTC
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
Comment 19 Gerald Pfeifer freebsd_committer 2021-11-07 08:49:02 UTC
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?
Comment 20 Graham Perrin 2021-11-07 10:52:43 UTC
(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.
Comment 21 Damjan Jovanovic 2021-12-03 05:28:22 UTC
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.
Comment 22 Gerald Pfeifer freebsd_committer 2021-12-03 20:19:05 UTC
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.