Skip to content

NSData: Refactor write(toFile:options) using FileHandle methods. #1876

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 12, 2019
Merged

NSData: Refactor write(toFile:options) using FileHandle methods. #1876

merged 1 commit into from
Feb 12, 2019

Conversation

@spevans
Copy link
Contributor

@spevans spevans commented Feb 2, 2019 •
edited
Loading

  • Remove makeTemporaryFile(inDirectory:) in favour of using
    _NSCreateTemporaryFile(path).

  • FileHandle: Add internal _write(buf:length) method to write out memory
    buffers.

  • FileHandle: Add a throwing _synchronizeFile(), use it in synchronizeFile().

@spevans
Copy link
Contributor Author

spevans commented Feb 2, 2019

@swift-ci test

@spevans
Copy link
Contributor Author

spevans commented Feb 2, 2019

CC @compnerd @phausler @millenomi

This is the first FileHandle refactor, to simplify NSData.write(). It needs a 2nd PR to make FileManager.setAttributes(:ofItemAtPath) and FileManager.attributesOfItem(atPath:) work on Windows and there will be another FileHandle one to tidy up the other functions a bit.

@compnerd
Copy link
Member

compnerd commented Feb 2, 2019

This definitely seems to be cleaning up a bunch of stuff. At the very least, some of this stuff is NFC, and you could split it off and merge those earlier (e.g the checkFileHandle and fileDescriptor variable refactorings).

@spevans spevans mentioned this pull request Feb 2, 2019
Merged
@spevans
Copy link
Contributor Author

spevans commented Feb 3, 2019

@swift-ci test

@spevans spevans mentioned this pull request Feb 3, 2019
Closed
@spevans
Copy link
Contributor Author

spevans commented Feb 4, 2019

@swift-ci test

compnerd
compnerd previously requested changes Feb 5, 2019
View reviewed changes
Foundation/FileHandle.swift Outdated
if WriteFile(handle, buf.advanced(by: length - bytesRemaining), DWORD(bytesRemaining), &bytesWritten, nil) == FALSE {
throw _NSErrorWithErrno(Int32(GetLastError()), reading: false, path: nil)
}
if BytesWritten == 0 {
Copy link
Member

@compnerd compnerd Feb 5, 2019

Choose a reason for hiding this comment

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

This should be bytesWritten

Copy link
Contributor Author

@spevans spevans Feb 5, 2019

Choose a reason for hiding this comment

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

Good catch - fixed.

@spevans
Copy link
Contributor Author

spevans commented Feb 5, 2019

@swift-ci test

1 similar comment
@spevans
Copy link
Contributor Author

spevans commented Feb 5, 2019

@swift-ci test

@compnerd compnerd dismissed their stale review February 6, 2019 01:51

Issue fixed

@compnerd
Copy link
Member

compnerd commented Feb 6, 2019

Alright, this looks good to me. Not sure if @parkera @phausler @millenomi have had a chance to look this over yet.

@compnerd
Copy link
Member

compnerd commented Feb 7, 2019

@parkera @phausler @millenomi - this is actually now blocking work on the Windows port.

@millenomi
Copy link
Contributor

millenomi commented Feb 8, 2019 •
edited
Loading

Please rebase this on the new API once that lands? There's a synchronize(), and _write can move there replacing the NSData calls in both write(_:) and write(contentsOf:).

@millenomi
Copy link
Contributor

millenomi commented Feb 8, 2019

LGTM once rebased!

@spevans
NSData: Refactor write(toFile:options) using FileHandle methods.
bb74cad 
- Remove makeTemporaryFile(inDirectory:) in favour of using
  _NSCreateTemporaryFile(path).

- FileHandle: Add internal _writeBytes(buf:length) method to write out
  memory buffers.

- FileManager: add _permissionsOfItem(atPath:) to only get permissions
  as it does less work than attributesOfItem(atPath:).
@spevans
Copy link
Contributor Author

spevans commented Feb 12, 2019

@swift-ci test

@spevans
Copy link
Contributor Author

spevans commented Feb 12, 2019

@millenomi This has been rebased on top of the new FileHandle API.

@millenomi
Copy link
Contributor

millenomi commented Feb 12, 2019

LGTM

@millenomi millenomi merged commit 87517d4 into swiftlang:master Feb 12, 2019
@niw niw mentioned this pull request Jul 28, 2020
Merged
niw added a commit to niw/swift-corelibs-foundation that referenced this pull request Aug 10, 2020
@niw
Allow Foundation to create file with permissions respects process `um…
cabbc54 
…ask`

**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `uamask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
creating a file with 0600 permission, if the system follows
the latest POSIX specification or the permission is undefined.

On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
`mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

It uses `0o600` even if it uses `open(2)` that respects `umask`.
Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

JIRA: https://bugs.swift.org/browse/SR-13307
niw added a commit to niw/swift-corelibs-foundation that referenced this pull request Aug 10, 2020
@niw
Allow Foundation to create file with permissions respects process `um…
7aadbcd 
…ask`

**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `uamask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
creating a file with 0600 permission, if the system follows
the latest POSIX specification or the permission is undefined.

On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
`mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

It uses `0o600` even if it uses `open(2)` that respects `umask`.
Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

JIRA: https://bugs.swift.org/browse/SR-13307
niw added a commit to niw/swift-corelibs-foundation that referenced this pull request Aug 10, 2020
@niw
Create a file with permissions that respects umask
82244a7 
**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `uamask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
creating a file with 0600 permission, if the system follows
the latest POSIX specification or the permission is undefined.

On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
`mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

It uses `0o600` even if it uses `open(2)` that respects `umask`.
Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

JIRA: https://bugs.swift.org/browse/SR-13307
@niw niw mentioned this pull request Aug 10, 2020
Merged
niw added a commit to niw/swift-corelibs-foundation that referenced this pull request Aug 25, 2020
@niw
Create a file with permissions that respects umask
d01bd2f 
**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `umask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

    It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
    creating a file with 0600 permission, if the system follows
    the latest POSIX specification or the permission is undefined.

    On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
    `mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

    It uses `0o600` even if it uses `open(2)` that respects `umask`.
    Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

Swift JIRA is https://bugs.swift.org/browse/SR-13307.
@niw niw mentioned this pull request Aug 25, 2020
Merged
niw added a commit to niw/swift-corelibs-foundation that referenced this pull request Aug 26, 2020
@niw
Create a file with permissions that respects umask
caddcd4 
**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `umask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

    It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
    creating a file with 0600 permission, if the system follows
    the latest POSIX specification or the permission is undefined.

    On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
    `mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

    It uses `0o600` even if it uses `open(2)` that respects `umask`.
    Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

Swift JIRA is https://bugs.swift.org/browse/SR-13307.
niw added a commit to niw/swift-corelibs-foundation that referenced this pull request Aug 28, 2020
@niw
Create a file with permissions that respects umask
a07c193 
**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `umask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

    It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
    creating a file with 0600 permission, if the system follows
    the latest POSIX specification or the permission is undefined.

    On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
    `mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

    It uses `0o600` even if it uses `open(2)` that respects `umask`.
    Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

Swift JIRA is https://bugs.swift.org/browse/SR-13307.
valeriyvan pushed a commit to valeriyvan/swift-corelibs-foundation that referenced this pull request Sep 12, 2020
@niw @valeriyvan
Create a file with permissions that respects umask
f1ff2ae 
**Problem**

`Data.write(to:)` is a only method in the Foundation that can create a
regular file.
However, it ignores `umask` and always set 0600 permission unlike
macOS Foundation, which respects process `umask`.

**Solution**

1. With `.atomic` write option

    It uses `mkstemp(3)` in `_NSCreateTemporaryFile`, which is always
    creating a file with 0600 permission, if the system follows
    the latest POSIX specification or the permission is undefined.

    On macOS Foundation, therefore `_NSCreateTemporaryFile` uses
    `mktemp(3)` and `open(2)` instead to respect `umask`.

2. Without `.atomic` write option

    It uses `0o600` even if it uses `open(2)` that respects `umask`.
    Simply gives `0o666` instead.

This is a bug caused by previous commit in
swiftlang#1876.

Swift JIRA is https://bugs.swift.org/browse/SR-13307.
@niw niw mentioned this pull request Jul 28, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compnerd compnerd compnerd left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@spevans @compnerd @millenomi