Bug 12436 - Fix the last resort check that sets the file type attribute
Summary: Fix the last resort check that sets the file type attribute
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: 12261
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-18 16:18 UTC by Ralph Böhme
Modified: 2016-12-02 08:02 UTC (History)
3 users (show)

See Also:


Attachments
Patch for master (1.08 KB, patch)
2016-11-18 16:22 UTC, Ralph Böhme
jra: review-
Details
git-am fix for master. (1.37 KB, patch)
2016-11-18 18:26 UTC, Jeremy Allison
slow: review-
Details
Patch for master (1.42 KB, patch)
2016-11-18 21:17 UTC, Ralph Böhme
jra: review+
Details
Patch for 4.4 and 4.5 cherry-picked from master (1.69 KB, patch)
2016-11-30 17:29 UTC, Ralph Böhme
jra: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2016-11-18 16:18:46 UTC
The fix in #12261 for setting file type is dos_mode() in case SMB_VFS_GET_DOS_ATTRIBUTES() didn't set neither FILE_ATTRIBUTE_NORMAL nor FILE_ATTRIBUTE_DIRECTORY.

The last resort check was only triggered if SMB_VFS_GET_DOS_ATTRIBUTES() returned zero, so if it returned eg FILE_ATTRIBUTE_ARCHIVE we still fail to set the file type attribute.... d'oh!

Patch to follow....
Comment 1 Ralph Böhme 2016-11-18 16:22:54 UTC
Created attachment 12673 [details]
Patch for master
Comment 2 Ralph Böhme 2016-11-18 16:24:29 UTC
Jeremy, I didn't get it right the first time, please help me to get it right with the second attempt. Sorry! :/
Comment 3 Jeremy Allison 2016-11-18 17:07:50 UTC
Bugger, looks like I didn't get the review right either (and it looked a completely simple fix !). Doh. I'll take a look asap.
Comment 4 Jeremy Allison 2016-11-18 18:19:03 UTC
Comment on attachment 12673 [details]
Patch for master

This patch breaks samba3.base.delete. I'll upload a fixed patch shortly.
Comment 5 Jeremy Allison 2016-11-18 18:26:29 UTC
Created attachment 12674 [details]
git-am fix for master.

Ralph, I think this is the correct fix. What your patch got wrong is that a directory must always have FILE_ATTRIBUTE_DIRECTORY set - no matter what other attributes are on the directory, but a file only has FILE_ATTRIBUTE_NORMAL set if there are *NO* other attributes set. Once a file has hidden, system, etc. set then we do not add FILE_ATTRIBUTE_NORMAL.

This fix passes samba3.base.delete.

Please review and push to master if happy.

Jeremy.
Comment 6 Ralph Böhme 2016-11-18 21:16:03 UTC
Comment on attachment 12674 [details]
git-am fix for master.

Oh, glad you caught this!

I believe your commit message then is not quite right and doesn't match the code. I have an update patch with a rewritten commit message and otherwise unchanged code.
Comment 7 Ralph Böhme 2016-11-18 21:17:18 UTC
Created attachment 12677 [details]
Patch for master
Comment 8 Jeremy Allison 2016-11-18 21:28:09 UTC
Comment on attachment 12677 [details]
Patch for master

LGTM. If you don't get it in first I'll add on my next autobuild (autobuild is currently failing for me).
Comment 9 Ralph Böhme 2016-11-30 17:29:01 UTC
Created attachment 12704 [details]
Patch for 4.4 and 4.5 cherry-picked from master
Comment 10 Jeremy Allison 2016-11-30 17:36:10 UTC
Comment on attachment 12704 [details]
Patch for 4.4 and 4.5 cherry-picked from master

LGTM.
Comment 11 Jeremy Allison 2016-11-30 17:36:38 UTC
Re-assigning to Karolin for inclusion in 4.5.next, 4.4.next.
Comment 12 Karolin Seeger 2016-12-01 10:17:10 UTC
(In reply to Jeremy Allison from comment #11)
Pushed to autobuild-v4-{4,5}-test.
Comment 13 Karolin Seeger 2016-12-02 08:02:51 UTC
(In reply to Karolin Seeger from comment #12)
Pushed to both branches.
Closing out bug report.

Thanks!