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

[Uri Simchoni]

Continuing from 36/56 with some minor comments, then responding to last
email. Everything else LGTM.

Thanks for bearing with me,
Uri.

[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}()

[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?

>          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" ?


... 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

>> [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?
- 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.

> 
>> [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.

>> [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".
> 
> good catch! Fixed in the updated patchset.
> 

OK

>> 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 function is only called for the case when fruit:resource=xattr which
> only works on Solaris, as it uses the Solaris xattr API (which supports xattrs
> of arbitrary size and the POSIX file handle base API) and it is indeed
> incomplete. Thanks for spotting this.
> 
> I added the code to handle fruit:resource=xattr on Solaris to the initial
> vfs_fruit version, but it is long a time ago since I last tested it.
> 
> It was broken in several places before this patchset and the patchset improves
> on this in some places but not all.
> 
> To be honest, I'm not aware of anyone using fruit:resource=xattr (on a Solaris
> (or derived) system), so lacking time to properly implement and test it I see
> two options:
> 
> - mark it as incomplete and broken (via a patch for the manpage on top)
> 
> - remove it
> 
> What do you think?
> 
Come the use case, and the resources to test it, this can be easily
fixed if necessary. The benefits of resource:xattr are obvious.

>> 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.
> 
> How did you spot that one?! :) Fixed in updated patchset.
> 
OK

>> [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?