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

[Uri Simchoni]

Everything RB+ me, including the additional 2-patch set for bug 12565
and the 1-patch for bug 12526, except:

1. The catia caching patch needs some 80-col fixes (catia_fset_nt_acl()
really stands out...)

2. A peculiar thing about catia - if this needs fixing maybe it should
go on a follow-up patch because some of this is before the current patch
- the catia xattr handling seems a bit inconsistent:

catia_fgetxattr / catia_fsetxattr map both path and attr name
catia_fremovexattr maps only the path
catia_flistxattr / catia_listxattr map the path but do not reverse-map
the EA names
catia_getxattr / catia_removexattr / catia_setxattr map only the attr name

Is mapping EA names (which are not file names) in the scope of vfs_catia
at all?

Also, the catia readdir doesn't reverse-map the entries (pre-patch), so
I'm really getting confused now...

Thanks,
Uri.

On 02/22/2017 01:51 PM, Ralph Böhme wrote:
> Hi Uri,
> 
> sorry for the delay.
> 
> So here's an updated patchset with the changes described below as well as the
> catia translation cache discussed.
> 
> The catia translation cache patch already carries mine and metzes signed-off so
> it would be good to go, but you're very welcome to take a look as well.
> 
> Commits are slightly reordered, moving all non-fruit VFS module changes to the
> front of the patchset.
> 
> I've attached a simple diff showing just the differences in vfs_fruit.c between
> the last patchset and the new one.
> 
> Additional changes:
> 
> - fixed error handling in fruit_open_meta_stream (see fruit-changes.diff)
> 
> - extended existing test in commit "s4/torture: vfs_fruit: add stream with
>   illegal ntfs characters to copyile test" to test for something I came across
>   when developing the catia caching
> 
> - new commit "vfs_streams_xattr: call SMB_VFS_OPEN with smb_fname_base" fixes a
>   bug in this module
> 
> - new commit "vfs_streams_xattr: use SMB_VFS_NEXT_OPEN and CLOSE", there's no
>   need for recursive VFS open calls
> 
> On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
>> Continuing from 36/56 with some minor comments, then responding to last
>> email. Everything else LGTM.
>>
>> Thanks for bearing with me,
>> Uri.
> 
> thanks for taking the stab! :)
> 
>> [39/56]
>>> -static int fruit_fstat_rsrc(vfs_handle_struct *handle, files_struct *fsp,
>>> +static int fruit_fstat_meta(vfs_handle_struct *handle,
>>> +                           files_struct *fsp,
>>>                             SMB_STRUCT_STAT *sbuf)
>>>  {
>>> -       struct fruit_config_data *config;
>>> -       struct adouble *ad = (struct adouble *)VFS_FETCH_FSP_EXTENSION(
>>> -               handle, fsp);
>>> +       struct fio *fio = (struct fio *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
>>> +       int ret;
>>>  
>>> -       DEBUG(10, ("fruit_fstat_rsrc called for %s\n",
>>> -                  smb_fname_str_dbg(fsp->base_fsp->fsp_name)));
>>> +       if (fio == NULL) {
>>> +               return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
>>> +       }
>>
>> That (fio not being NULL) is already checked in fruit_fstat(). Maybe for
>> clarity and efficiency it would be better to pass fio to
>> fruit_fstat_{rsrc,meta}()
> 
> Done.
> 
>> [44/56], [45/56]
>>> +[vfs_fruit_metadata_stream]
>>> +       path = $shrdir
>>> +       vfs objects = fruit streams_xattr acl_xattr
>>
>> Shouldn't we have xattr_tdb below acl_xattr, to be env-agnostic?
> 
> Well, the existing vfs_fruit share didn't use xattr_tdb and I just copied. There
> are actually insane tests that use vfs_fruit that make direct filesystem access
> to the --option=torture:localdir provided path making direct xattr calls
> (test_read_netatalk_metadata).
> 
> In theory the new shares vfs_fruit_metadata_stream and vfs_fruit_stream_depot
> could use xattr_tdb as they don't use such hack. I just tried it but got some
> test failures and I really, really would prefer not to debug issues caused by
> xattr_tdb this time.
> 
>>
>>>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit -U$USERNAME%$PASSWORD --option=torture:localdir=$SELFTEST_PREFIX/nt4_dc/share')
>>> +        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit_metadata_stream -U$USERNAME%$PASSWORD')
>>
>> I didn't understand why localdir is not set - that skips tests, but I
>> couldn't figure out why they should be skipped.
>>
>> [50/56]
>>> +               return false; \
>>
>> Should be "ret = false" ?
> 
> Oh, good catch, thanks!
> 
>> ... And responses to previous patches:
>>
>> On 01/31/2017 01:56 PM, Ralph Böhme wrote:
>>> Hi Uri,
>>>
>>> thanks a lot for looking into this! Comments below:
>>>
>>> On Sun, Jan 29, 2017 at 09:13:41PM +0200, Uri Simchoni wrote:
>>>> 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.
>>>>
>>>> [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.
>>>
>>> agreed, fixed.
>>>
>> I still don't see the fix in last patch set
> 
> uh, sorry, no idea how that one got lost. :/ Fixed.
> 
> I've changed a few additional places to consistenly call the NEXT function and
> not recurse in the VFS.
> 
>>>> [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.
>>>
>>> Not quite. :) In all cases execept the Netatalk case the btime will also be set
>>> and fetched by the client (!) via the usual SMB protocol semantics and Samba FSA
>>> processing. But that's not the case with Netatalk. There the primary source for
>>> bime is the Netatalk metadata xattr, updated in fruit_ntimes() and queried here
>>> in update_btime().
>>>
>> I still don't understand :)
>>
>> - How does the client (mac or otherwise) know if the backend is netatalk
>> or something different?
> 
> it doesn't.
> 
>> - I understand that for Mac clients, it's not necessary to
>> update_btime() at all, since the client would read the metadata stream
>> and figure this by itself. So, if I understand correctly, what
>> update_btime() does is take care of things for non-Mac clients.
> 
> This is for the case where you have both Mac clients connecting over
> AFP/Netatalk *and* over SMB/Samba. update_btime() together with fruit_ntimes()
> ensures that the btime is consistently read and written from the Netatalk
> metadata xattr.
> 
>>>> [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.
>>>
>>> This comments describes OS X SMB *server* behaviour. That one ignores unlink
>>> requests on the resource fork (no idea at which layer, may just be the OS X SMB
>>> server FSA-layer that exposes this behaviour).
>>>
>> What I understood from that comment was that we do nothing *because* the
>> "stream" (implemented by xattr) is removed with the file, whereas the
>> real reason is that we act like an OS-X server.
> 
> Ah, *now* I see what you mean. :) You're right, I was mixing up
> comments. Attached patchset has an updated comment for this function.
> 
>>>> [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.
>>>
>>> And they get the fix is well -- which this is all about. Not doing the
>>> translation on the names attached to handles is just a bug.
>>>
>> ...not necessarily - handle-based APIs usually don't need paths - the
>> handle embodies the object. The cost of translating a path for each read
>> block might not be negligible. So this change could affect performance
>> for current catia users which don't use fruit.
>>
>> An easy alternative is to have those added functions depend on a
>> configuration var.
>>
>> Do we have vfs_catia stakeholders that can comment on this?
> 
> Attached patchset adds translated name caching via FSP extension.
> 
> Cheerio!
> -slow
>