[Linux] Implement update methods on Linux for placeholder files/dirs#1128
Conversation
5288077 to
47fb515
Compare
7d00a17 to
dce8325
Compare
|
/azp run PR - Linux - Build and Unit Test |
|
No pipelines are associated with this pull request. |
47fb515 to
972f05a
Compare
9612110 to
2b77aab
Compare
kivikakk
left a comment
There was a problem hiding this comment.
Looks great! Thank you 👍
The libprojfs API sets attribute sizes to the appropriate length of the actual value read, and to -1 if the attribute is not found.
2b77aab to
1f2cd71
Compare
derrickstolee
left a comment
There was a problem hiding this comment.
This looks fine to me. I ran a Linux build as a sanity check.
For the Linux implementation of the equivalent of the ProjFS.Mac DeleteFile() method, we rely on the projfs_get_attrs() function in the libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file is not fully local, and may be deleted; otherwise, we return an error.
1f2cd71 to
ac974c7
Compare
547c3c5 to
5d9c233
Compare
By using native rmdir(2) syscalls when deleting placeholder directories, we can specifically detect the non-empty directory error condition ENOTEMPTY, which is otherwise folded into a generic IOException by Directory.Delete(). We need to return EDirectoryNotEmpty in this case because the calling code in GitIndexProjection handles this result as an expected and non-exceptional condition. To simplify our code we then also use unlink(2) to delete files in a common NativeMethods subclass. We further detect the case where an inode is neither a file nor a directory (libprojfs's API will return EPERM for such inodes) by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, in the same manner as the macOS PrjFSLib C++ implementation. Finally, we handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short- circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection, which otherwise would fail to correctly populate a new view of the repository if we return Success when asked to delete the repository's root. Since the repository's root directory is expected to always contain, at a minimum, the .git subdirectory, EDirectoryNotEmpty is appropriate.
5d9c233 to
18a1486
Compare
|
Hi @kivikakk and @derrickstolee -- would you mind taking a second look at this PR, now that I've ended up making some moderately significant changes (in 18a1486)? In summary: from studying the calling context for #1160, I realized we really should be returning For the same reason, we also return My apologies for the extra go-round of another review cycle. |
The All looks good otherwise, though I'd slightly prefer using the framework methods over libc where possible. |
|
Me too, @kivikakk -- that is, I had originally written this using I did find one example in the existing GVFS codebase of non-Windows code testing an exception's I'm honestly not sure if it would make this code more or less readable/maintainable to go back to |
We can return to using managed methods to delete files and directories when updating projection views on Linux, so long as we check the IOException's HResult property to detect the EDirectoryNotEmpty (ENOTEMPTY) condition. Also set the (unused?) failure cause of ReadOnly when an access exception occurs. Thanks to @kivikakk for the hint on checking HResult!
|
Hi @kivikakk and @derrickstolee -- I think third time might be a charm on this PR. Would you mind doing a once-over and re-approving if it looks OK? Thanks so much and apologies for all the churn. |
Co-authored-by: Ashe Connor <ashe@kivikakk.ee>
|
For reference, @derrickstolee, here's what's changed: |
derrickstolee
left a comment
There was a problem hiding this comment.
LGTM. Sorry for the review delay.
|
There's no delay to apologize for, @derrickstolee -- I only finished this up last night! Thanks for the fast review! |
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
[Linux] Implement update methods on Linux for placeholder files/dirs For the Linux implementation of the equivalent of the PrjFSLib.Mac DeleteFile(), UpdatePlaceholderIfNeeded(), and ReplacePlaceholderFileWithSymLink() methods, we rely on the projfs_get_attrs() function in our libprojfs library, which allows us to read the "user.projection.empty" xattr. If this xattr exists, the file or directory is not fully local, and may be deleted. Our implementation of these functions follows the logic in the Mac C++ code, including the small races in the DeleteFile() method between performing a stat(2) to check the nature of the file/dir, then a getxattr(2) to look for our projection flag xattr, and finally a File.Delete() or Directory.Delete(). We detect the case where an inode is neither a file nor a directory by introducing an Unknown projection state and reporting this as a "full file" condition to our caller, again parallel to the Mac C++ code. We handle requests to delete the repository's root directory by returning EDirectoryNotEmpty, instead of ignoring them and returning Success. This is necessary to avoid short-circuiting the logic of the calling UpdatePlaceholders() method in GitIndexProjection. Separately, we ensure Linux platform CI builds succeed by ignoring the Profiling(Release) builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.
For the Linux implementation of the equivalent of the
PrjFSLib.Mac.DeleteFile()method (and PrjFS_DeleteFile() C++ function), we rely on theprojfs_get_attrs()function in our libprojfs library, which allows us to read theuser.projection.emptyxattr. If this xattr exists, the file or directory is not fully local, and may be deleted; otherwise, we return an error.Our implementation of these functions follows the logic in the Mac
PrjFSLib.cppcode, including the small races in theDeleteFile()method between performing astat(2)to check the nature of the file/dir, then agetxattr(2)to look for our projection flag xattr, and finally anunlink(2)orrmdir(2). (Note that unlike the Mac C++ implementation, here these low-level syscalls are actually performed in either libprojfs or in the C# standard library; they are just wrapped inside our calls toProjFS.GetProjState(),File.Delete(), etc.)Although we could try for a "more atomic" implementation by moving the code into libprojfs and acquiring a
flock(2), after consultation with @wilbaker this seems excessive, given that Git itself performs stat-followed-by-delete when trying to avoid deleting locally modified files, which is all this code is doing too.Separately, we ensure Linux platform CI builds succeed by ignoring the
Profiling(Release)builds (as done for Mac as well) and searching the usual system locations for our libprojfs headers and library.Along with
#1104,#1119, and#1123(ALL DONE), this PR is a prerequisite for theGVFS.Platform.Linuximplementation in #1125.Will resolve github#13.