Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mac PrjFSLib: Use IOSharedDataQueue workaround library if available #429

Merged
merged 1 commit into from
Oct 28, 2018

Conversation

pmj
Copy link
Contributor

@pmj pmj commented Oct 25, 2018

This allows the Mac PrjFSLib native library and prjfs-log tool to use fixed versions of the IODataQueueDequeue() and IODataQueuePeek() functions which suffer from severe regressions in macOS 10.13.6 and 10.14. (Darwin 17.7.0 and 18.0)

Assuming you have a dynamic library containing alternate versions of the aforementioned functions and place it in the library search path as "libSharedDataQueue.dylib", this change will cause PrjFSLib and prjfs-log load that library and use the versions of the aforementioned functions it provides instead of the OS's IOKit.framework's.

@pmj
Copy link
Contributor Author

pmj commented Oct 25, 2018

Previous discussion is at #161. For now this only deals with the userspace regression, which is vastly more severe than the kernel-side one. I can only rarely (< 1 in 10 million calls) trigger the kernel-side one very occasionally in synthetic tests that try to cause, so assuming it's fixed in 10.14.1, it's probably not worth spending much effort on.

@pmj pmj requested review from sanoursa and nickgra October 25, 2018 19:36
@pmj pmj force-pushed the ioshareddataqueue-workaround-dylib branch from a997ebd to d5a36b7 Compare October 25, 2018 19:41
{
utsname unameInfo = {};
if (0 != uname(&unameInfo))
{
Copy link
Contributor Author

@pmj pmj Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unfortunately not aware of a nicer way to get the macOS version with just the C system libraries. (as opposed to Objective-C/Cocoa) There's also a "kern.osrelease" sysctl, but that returns the exact same version string as uname() that still needs to be parsed. Alternative suggestions welcome.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider sw_vers -productVersion? That should give you the macOS version number, not Darwin. You'd still have to do some parsing though.

Copy link
Contributor Author

@pmj pmj Oct 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require shelling out or reading the file /System/Library/CoreServices/SystemVersion.plist and subsequently parsing the string anyway as you say. I think a simple syscall is preferable in that regard. The Darwin version tends to change on very minor revisions (security updates, supplemental updates, etc.) too, so in theory that would allow us to turn off the workaround if Apple changed their mind and supplied a fix with a supplementary update to 10.13.6. (For example, I think there was a supplemental update to 10.13.3, which would explain why 10.13.6 is Darwin 17.7, not 17.6.)

@nickgra
Copy link
Member

nickgra commented Oct 25, 2018

Thanks @pmj for putting this together. We're having some packaging issues at the moment, but once that's done, I'll get your repo packaged up and set up a PR to consume it. Do you want this PR to go in first?

@pmj
Copy link
Contributor Author

pmj commented Oct 26, 2018

Do you want this PR to go in first?

I wouldn't mind getting this into the tree now, as it's one fewer local patch I need to keep maintaining - and the workaround is pretty much essential for preventing the log from seizing up during development/testing, so it should be useful not just for me. Until the dylib is packaged, we can just build it manually and place it in the same directory as prjfs-log.

Copy link
Member

@nickgra nickgra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

This allows PrjFSLib and the prjfs-log tool to use alternate versions of the IODataQueueDequeue() and IODataQueuePeek() functions from a dylib on OS versions where the built-in functions are buggy. On affected versions, a library "libSharedDataQueue.dylib" is loaded if available, and its implementations of the aforementioned functions are used for the message and kext logging queues instead of the ones supplied by the OS's IOKit.framework.
@pmj pmj force-pushed the ioshareddataqueue-workaround-dylib branch from d5a36b7 to c7e221b Compare October 27, 2018 19:22
@pmj pmj merged commit 920fe27 into microsoft:master Oct 28, 2018
@pmj pmj deleted the ioshareddataqueue-workaround-dylib branch October 28, 2018 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants