Hi Eric
Eric Biggers wrote:
> 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
Updated now (same location)
>> 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.
Done (IMHO).
> - The 'size' argument to truncate() should be off_t.
Done.
> - The documentation for init() should mention the errno to set (currently
> EINVAL
> in your proposal) if the reparse tag is not supported.
Done.
> - 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.
Done. However this is done uncleanly (has always
been), because ntfs_ucstombs() has no access to the
volume description which could store the locale option.
Anyway not using utf8 is discouraged...
> - 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.
Done.
> 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
Maybe, but ENODATA is defined by Posix as optional...
> be confused with ntfs_get_ntfs_reparse_data(). Maybe it would be better to
> call
> it ntfs_get_reparse_point()?
Done.
> - 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;
> }
Done.
> 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;
> }
Not done (yet ?). There are more exceptions now.
I agree this should be improved, but have to find
out how.
> - Perhaps it should be possible to disable external plugins at build time as a
> ./configure option?
Done. This obfuscates the code even more.
> - 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/.
There are at least three problems with this :
- if I force a new option on ./configure, some
distributions will give up. There are distros
which are still releasing versions from before
the ntfsprogs were merged into ntfs-3g.
- AFAIK the standard library directory is not known
at compile time, and I do not known how to get it
at execution time.
- if the plugin directory is defined as an absolute
path in a configure option, I cannot test a new
release without installing it (or executing some
specific code in test mode, like a car manufacturer)
Unless a better way is found, ./configure with no
new option will not enable the reparse plugins.
> - 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.
Done.
> - 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.
Fixed.
> - release() should probably be a no-op if it isn't provided rather than
> failing with -EOPNOTSUPP.
Done.
> - 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?
Done.
> - 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.
Debatable : the head of the list is in the context
used by ntfs-3g.c
Jean-Pierre
------------------------------------------------------------------------------
_______________________________________________
ntfs-3g-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/ntfs-3g-devel
