Hi,
On Fri, Dec 04, 2015 at 10:03:41AM +0100, Jean-Pierre André wrote:
> Hi Eric,
>
> Please see http://jp-andre.pagesperso-orange.fr/systcomp.tar.gz
> with (most of) your comments taken into account.
It generally looks good. I did a basic test of the system compression plugin
(reading files only) and it worked correctly. Here are some comments on the
code:
- I think there should be a comment in plugin.h that describes the reparse
plugin architecture and when the header is needed. It should clarify that the
header is there for plugin development for the FUSE drivers and is not part of
the libntfs-3g API.
- The 'size' argument to truncate() should be off_t.
- The documentation for init() should mention the errno to set (currently EINVAL
in your proposal) if the reparse tag is not supported.
- The documentation for readlink() says the link target must be encoded in
UTF-8,
but I don't believe that's necessarily true because NTFS-3g supports alternate
locales. The link target will need to be returned as a "multibyte string" as
provided by ntfs_ucstombs(), which is what ntfs_make_symlink() does.
- ntfs_get_reparse_data() needs a comment to document it. It should note that
the
return value, if not NULL, has already been validated as a proper reparse point.
Also, there is the case where the file is not a reparse point. It looks like the
function would fail with ENOENT in that case; is that the proper error code or
should it be something else like ENODATA? Another issue is that it is prone to
be confused with ntfs_get_ntfs_reparse_data(). Maybe it would be better to call
it ntfs_get_reparse_point()?
- There is a lot of boilerplate code around calling the reparse point
operations. I have two ideas for improvement. First, there could be a function
that combines ntfs_get_reparse_data() and get_reparse_plugin():
const struct plugin_operations *select_reparse_plugin(ntfs_inode *ni,
ntfs_fuse_context_t *ctx,
REPARSE_POINT
**reparse_ret)
{
REPARSE_POINT *reparse;
const struct plugin_operations *ops;
reparse = ntfs_get_reparse_data(ni);
if (!reparse)
return NULL;
ops = get_reparse_plugin(ctx, reparse->reparse_tag);
if (ops)
*reparse_ret = reparse;
else
free(reparse);
return ops;
}
Second, there could be a macro which calls a plugin operation:
#define CALL_REPARSE_PLUGIN(ni, op_name, ...) \
({ \
const struct plugin_operations *ops; \
REPARSE_POINT *reparse; \
int res; \
\
ops = select_reparse_plugin(ni, ctx, &reparse); \
if (ops) { \
if (ops->op_name) \
res = ops->op_name(ni, reparse, ##__VA_ARGS__); \
else \
res = -EOPNOTSUPP; \
free(reparse); \
} else { \
res = -errno; \
} \
res; \
})
Maybe there would be too much magic going on behind the scenes with the macro,
but it does get rid of the boilerplate code. Example for truncate():
if (ni->flags & FILE_ATTR_REPARSE_POINT) {
if (stream_name_len) {
res = -EINVAL;
goto exit;
}
res = CALL_REPARSE_PLUGIN(ni, truncate, size);
if (res)
goto exit;
set_archive(ni);
goto stamps;
}
- Perhaps it should be possible to disable external plugins at build time as a
./configure option?
- In the final version, I think there should be a dedicated directory created
for plugins. It's not really appropriate to drop plugins in the top-level
system library directory. Probably the plugin directory should be settable
by ./configure and should default to something like ${libdir}/ntfs-3g/.
- The function names "set_reparse_plugin()" and "set_internal_reparse_plugins()"
seem a little nonstandard. I think they should use the verb "register" instead
of "set". You're "registering" a plugin, not "setting" a plugin.
- Interesting idea with the fi->fh value. I'll have to see if I can do anything
with that for system compression. I am not sure it's possible to keep an
attribute open across reads(), but it probably would at least be possible to
keep some information about the compressed file cached in memory.
I did notice an inconsistency, however: it looks like lowntfs-3g.c calls
release() if fh is left as 0 whereas ntfs-3g.c doesn't call it.
- release() should probably be a no-op if it isn't provided rather than
failing with -EOPNOTSUPP.
- I assume that 'ntfs_bad_reparse' (the string "unsupported reparse point")
should be kept around for invalid symlinks and junctions, even though it doesn't
get used for other reparse points (with other tags) anymore?
- Perhaps plugin_list_t should be made private to src/ntfs-3g_common.c? I don't
think other files should care about how the list is implemented. The function
to register a new plugin can return 0 on success or -1 with errno set on
failure.
Eric
------------------------------------------------------------------------------
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=254741911&iu=/4140
_______________________________________________
ntfs-3g-devel mailing list
ntfs-3g-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
