Hi Eric, Thank you for your reviewing.
Eric Biggers wrote: > On Mon, Nov 23, 2015 at 08:49:44AM +0100, Jean-Pierre André wrote: >> See proposal on >> http://jp-andre.pagesperso-orange.fr/systcomp.patches.gz > > Hi, > > Great work! It looks cleaner than I had expected. I have a few questions and > comments: > > Should plugin_operations_t really be declared in reparse.h? reparse.h is > supposed to be a header for the library, but the plugins are specific to the > FUSE driver. Agreed. It should be plugin.h, but where should this be located ("src/plugin.h" ?) > I think the plugins will need to always be loaded from a fixed location set > during the build. The 'launch_path' logic effectively allows users to load > arbitrary plugins into ntfs-3g, which is a problem if someone has ntfs-3g > installed setuid root. Agreed, the "launch_patch" will be removed. It was useful for debugging, developers will have to set LD_LIBRARY_PATH > Naming: should the word "reparse" be included? For example: > "ntfs_reparse_plugin_operations_t", "ntfs_plugin_reparse_80000017.so". In the > code, just calling them "plugins" seems too vague. Debatable, "ntfs_plugin" is not so vague... > Of course, the way the > plugins are presented to users can still be feature-oriented, e.g. "this > NTFS-3g > plugin allows you to read system-compressed files". > > With a reparse plugin for tag 0x80000017 I will get sent all the WOF reparse > points, including WIM-backed files ("WIMBoot" files) and anything Microsoft > may > add to WOF in the future, not just system-compressed files. Microsoft advertises IO_REPARSE_TAG_WIM (0x80000008) Is that not used for WIMBoot files ? > On unsupported > files, should I just fail operations with EOPNOTSUPP? Or should I pretend the > file is a symlink to "unsupported reparse point"? Both are currently possible, but the second option implies defining a readlink(). IMHO WOF files are not supposed to be symlinks, and returning a symlink for a file of unknown format could be misleading. More generally, if a plugin does not define an entry, returning ENOTSUPP appears to be safer than trying to define a default action. > There is an edge case in ntfs_get_reparse_tag(). If the reparse tag is 0, the > function returns 0 but doesn't set errno. This could be fixed by having > valid_reparse_data() consider reparse tags of 0 to be invalid. layout.h > already > defines the constant IO_REPARSE_TAG_RESERVED_ZERO, and Windows considers this > value to be invalid as well. Agreed. > In the FUSE driver, there could be a reusable function which combines > ntfs_get_reparse_tag() and get_reparse_plugin(). Probably yes. > reparse_getattr() and reparse_readlink() sound overly generic, since they > don't > handle all types of reparse points. Maybe they should be renamed to > symlink_getattr() and symlink_readlink(). Or perhaps > link_reparse_point_getattr() and link_reparse_point_readlink()? I initially named them junction_getattr() and junction_readlink()... I would need a collective name for symlinks and junctions... > The reparse plugin operations need to be clearly documented so that plugin > authors know what to do. Agreed. > It probably makes the most sense to pass the reparse data to the plugin > operations. Otherwise, almost every operation would end up immediately > reading > the reparse data anyway. There would, however, need to be a new version of > ntfs_make_symlink() that takes in the reparse data directly. Ok. The ntfs_get_reparse_tag() will be replaced by ntfs_get_reparse_data(), and a "const char*" argument will be added to plugin calls. > With the proposed hook placement in ntfs_fuse_read(), there is no way for the > plugin to support reading named data streams. For system compressed files, > only > the default behavior is needed. So to fix this, it would be sufficient to > only > call the plugin if the unnamed data stream is being read. That may be fine, > and > ntfs_fuse_getattr() already has similar behavior. But I don't know whether > that > behavior is necessarily what all plugins will want. Supporting named data stream is not on offer for now, the named data stream do not have an inode number, so they cannot be managed properly in the vfs and the fuse interface, which leads to numerous issues (hard linking, cacheing, etc). The plugins will have to process files globally (and read() will be fixed). > There is an open() plugin operation, but isn't called from anywhere. It should be, of course. My plan is to have only seven entries initially : getstat(), open(), release(), read(), write(), readlink() and truncate(). > Do plugins need to export a "cleanup" or "deinit" function to go along with > "init"? Or should it be assumed that either plugins won't allocate global > resources or will use library destructors (__attribute__(cleanup) with gcc)? I thought this would not be useful. If a context is needed for open files, it can be hooked in the fuse context and freed on release(). Jean-Pierre ------------------------------------------------------------------------------ Go from Idea to Many App Stores Faster with Intel(R) XDK Give your users amazing mobile app experiences with Intel(R) XDK. Use one codebase in this all-in-one HTML5 development environment. Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs. http://pubads.g.doubleclick.net/gampad/clk?id=254741551&iu=/4140 _______________________________________________ ntfs-3g-devel mailing list ntfs-3g-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
