Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

[Uri Simchoni]

So far I got up to 35/56, although the catia patches deserve another
look. I have some minor comments, see below.
Will be looking at the rest, but it would take a few more days to get
back to - hopefully next weekend.

Thanks,
Uri.
On 01/12/2017 05:08 PM, Ralph Böhme wrote:
> Hi!
> 
> Attached is a patchset for vfs_fruit that fixes bug #12427.
> 
> This addresses that essentially any internal metadata backend other then the
> default was broken at one place or another.
> 
> Most part of the patchset is a proper refactoring of all VFS functions
> implemented in vfs_fruit to switch between the configured backend. I haven't yet
> taken the full step to fully modularize the backends, but this could be done
> later.
> 
> At then end, we get to fully test all possible backend configs in autobuild as
> well as a few additional tests for issues I found.
> 
> I had to disable and later reenabled vfs.fruit tests in the middle of the
> patchset in order to stay git-bisect clean. It wasn't possible to refactor the
> module without breaking something at some point, so I chose this approach.
> 
> I dare not ask, but can anyone please review this one and push if ok! I owe you
> a beverage of your choice at SambaXP! Thanks!
> 
> Cheerio!
> -slow
> 

[10/56] -
On error, shouldn't we be calling SMB_VFS_NEXT_CLOSE instead of
SMB_VFS_CLOSE? The choice between SMB_VFS_XXX and SMB_VFS_NEXT_XXX is
never an easy one. However, in the case of
close-from-within-failed-open, it's the file we've just opened, and
higher layers may have not seen the open return.

I also don't understand the BUGBUGBUG comment - fd_close_posix() *is*
called if it's a regular file and we get to the bottom of the vfs stack.

[13/56] -
That doesn't seem right... if we update btime out of the Mac metadata,
we do it no matter where the metadata is stored. One way is to call
fruit_open_meta(), read the metadata via fruit_pread_meta(), and parse
the metadata.

BTW, I was a bit puzzled by the use of update_btime() while stat'ing a
stream. What is the meaning of stat'ing a stream? stat is a UNIX concept
and a stream is a Windows (Mac too...) concept. Are there any "rules" of
what should a stat of a stream return?

[15/56] -
Seems like readdir_attr_meta_finderi_stream() would work for netatalk
too, since we know how to open and read the meta stream no matter where
it's being stored. Is the separate netatalk implementation a performance
optimization?

[17/56]

> +static int fruit_unlink_rsrc_xattr(vfs_handle_struct *handle,
> +                                  const struct smb_filename *smb_fname,
> +                                  bool force_unlink)
> +{
> +       /* Nothing to do here, removing the file will remove the xattr */
About this comment - If I understand correctly, we do nothing because
OS-X don't delete the resource.

[26/56]
in fruit_streaminfo_meta_netatalk():
> +       if (is_fi_empty) {
> +               return NT_STATUS_OK;
> +       }
> +
I think this early return doesn't let us remove the :$DATA "stream".

in fruit_streaminfo_rsrc_xattr():
Seems like the code assumes that someone already added the resource
stream, and we just have to filter it if it's empty. This might be the
case if we have streams_xattr below, but:
1. Streams_xattr could prvide us with a wrongly-named stream, especially
if streams_xattr:prefix is set
2. We may have another stream engine which doesn't tough extended
attributes.

[28/56]
> +static int fruit_ftruncate_rsrc_xattr(struct vfs_handle_struct *handle,
> +                                     struct files_struct *fsp,
> +                                     off_t offset)
> +{
> +       if (offset == 0) {
> +               return SMB_VFS_FREMOVEXATTR(fsp, AFPRESOURCE_EA_NETATALK);
> +       }
> +
> +       return SMB_VFS_NEXT_FTRUNCATE(handle, fsp, offset);
> +}
> +
For safety against future mistakes, and also for readability, I would
enclose the _NEXT call with #ifdef HAVE_ATTROPEN.


[30/56] and [31/56]:
Doing path translation inside a handle-based function raises some
warning flags inside my head. True, it's been that way before. But now,
other users of vfs_catia (assuming they exist :)) pay the toll as well.
The file name gets analyze on every pread().

Maybe you can cache the path during open to avoid this? Even as a
follow-up patch set...