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

saveconfig: open the temp configfile with modes set #162

Merged
merged 2 commits into from
May 28, 2020

Conversation

pkalever
Copy link
Contributor

@pkalever pkalever commented May 28, 2020

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>

Fixes: open-iscsi#161
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
@pkalever
Copy link
Contributor Author

Thanks to https://stackoverflow.com/a/15015748, which made my life easy.

shutil.copyfile() will not copy permissions, so all the perms that we
set on tempfile will go for a toss, and will be reset to default

┌──────────────────┬────────┬───────────┬───────┬────────────────┐
│     Function     │ Copies │   Copies  │Can use│   Destination  │
│                  │metadata│permissions│buffer │may be directory│
├──────────────────┼────────┼───────────┼───────┼────────────────┤
│shutil.copy       │   No   │    Yes    │   No  │      Yes       │
│shutil.copyfile   │   No   │     No    │   No  │       No       │
│shutil.copy2      │  Yes   │    Yes    │   No  │      Yes       │
│shutil.copyfileobj│   No   │     No    │  Yes  │       No       │
└──────────────────┴────────┴───────────┴───────┴────────────────┘

Without this fix:
----------------
$ ls /etc/target/saveconfig.json -l
-rw-r--r-- 1 root root 5078 May 28 20:01 /etc/target/saveconfig.json

With this fix:
--------------
$ ls /etc/target/saveconfig.json -l
-rw------- 1 root root 5078 May 28 20:15 /etc/target/saveconfig.json

Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
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