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

Jb/issue 1613 #1668

Merged
merged 2 commits into from
Jan 18, 2017
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions Frameworks/CoreFoundation/Preferences.subproj/CFPreferences.c
Original file line number Diff line number Diff line change
Expand Up @@ -182,19 +182,24 @@ static CFURLRef _preferencesDirectoryForUserHostSafetyLevel(CFStringRef userName
#if DEPLOYMENT_TARGET_WINDOWS

CFURLRef url = NULL;
Boolean success = false;

CFMutableStringRef completePath = _CFCreateApplicationRepositoryPath(alloc, CSIDL_APPDATA);
if (completePath) {
// append "Preferences\" and make the CFURL
CFStringAppend(completePath, CFSTR("Preferences\\"));
url = CFURLCreateWithFileSystemPath(alloc, completePath, kCFURLWindowsPathStyle, true);
Copy link
Member

Choose a reason for hiding this comment

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

completePath [](start = 47, length = 12)

what is this used for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @bbowman .

I also don't quite understand the issue and what the fix is here. Can you elaborate?

The changes I made are fix for the issue #1613 . In my last comment to the issue 1613 I mentioned that I found the solution, which I applied in "_preferencesDirectoryForUserHostSafetyLevel" in CFPreferences.c . But then I changed it to the implementation, which I submitted as PR, because I thought it was more relevant.

_CFCreateDirectory(CFStringGetCStringPtr(completePath, kCFStringEncodingUTF8)); // WINOBJC: ensure directory exists

char cPath[CFMaxPathSize];
Copy link
Member

Choose a reason for hiding this comment

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

CFMaxPathSize [](start = 12, length = 13)

what is this value and is it sufficient for all cases?

Copy link
Contributor Author

@jboich jboich Jan 11, 2017

Choose a reason for hiding this comment

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

Hi, @bbowman .
CFMaxPathSize defined in CFInternal.h as following:

`/* Buffer size for file pathname */
#if DEPLOYMENT_TARGET_WINDOWS
#define CFMaxPathSize ((CFIndex)262)
#define CFMaxPathLength ((CFIndex)260)`

I do not know why such value was chosen for Windows target. CFMaxPathSize definition used in many places of CoreFoundation.

if (CFURLGetFileSystemRepresentation(url, true, (unsigned char *)cPath, CFMaxPathSize)) {
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure any and all changes to CoreFoundation are annotated with a // WINOBJC:
comment. This helps more easily pinpoint differences from the reference platform implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please make sure any and all changes to CoreFoundation are annotated with a // WINOBJC:

will do, thank you

Copy link
Member

Choose a reason for hiding this comment

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

CFURLGetFileSystemRepresentation [](start = 5, length = 32)

looking at the implementation for this, it appears that it can return false if the buffer is too small as well. Not a lot we can do here about that though. I think the MaxPath thing is fine for now but it does worry me a for very long names.

success = _CFCreateDirectory(cPath);
Copy link
Member

@bbowman bbowman Jan 10, 2017

Choose a reason for hiding this comment

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

   [](start = 0, length = 2)

Please make sure to run clang-format before submitting any code. In this case you have tabs that should be spaces. Coding conventions are a sticking point for a lot of people so we make it pretty easy to ensure the code is conformant (https://github.com/Microsoft/WinObjC/wiki/ClangFormat) for more info.

EDIT: Just realized that core foundation turns off clang-format because we don't want to make unecessary churn from the ref platform. Can you just manually change your tabs to spaces? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you just manually change your tabs to spaces?

sure, will do

}

CFRelease(completePath);
}


// Can't find a better place? Home directory then?
if (url == NULL)
if (!success)
url = CFCopyHomeDirectoryURLForUser((userName == kCFPreferencesCurrentUser) ? NULL : userName);

return url;
Expand Down