SMB_VFS_GET_DOS_ATTRIBUTES vs SMB_VFS_IS_OFFLINE

[Ralph Böhme]

On Sun, Sep 11, 2016 at 09:23:21PM +0300, Uri Simchoni wrote:
> On 09/11/2016 08:39 PM, Ralph Böhme wrote:
> > 
> > On Sun, Sep 11, 2016 at 07:52:03PM +0300, Alexander Bokovoy wrote:
> >> On Sun, 11 Sep 2016, Ralph Böhme wrote:
> >>> Now that we have a full fledged SMB_VFS_{GET|SET}_DOS_ATTRIBUTES() I
> >>> was wondering whether it would be doable to merge SMB_VFS_IS_OFFLINE()
> >>> into SMB_VFS_GET_DOS_ATTRIBUTES()?
> >>>
> >>> Merging both functions would have the benefit that in VFS modules that
> >>> use same backing store bits for both, like gpfs, we would avoid
> >>> calling the backing store twice in dos_mode().
> >>>
> >>> I don't see a reason why SMB_VFS_{GET|SET}_DOS_ATTRIBUTES() couldn't
> >>> handle FILE_ATTRIBUTE_OFFLINE as well. WIP patch attached, passed a
> >>> private autobuild.
> >>>
> >>> Thoughts?
> >>
> >> It breaks ABI for VFS modules so an increase of VFS API number is
> >> required.
> > 
> > sure, WIP in the commit message and no sign-off where meant to flag
> > this as "of course some things are missing". :)
> > 
> >> Alternatively, don't delete offline functions and make calls
> >> to them from get/set functions in case they were set in the module. The
> >> latter would allow to keep ABI intact and recommend to avoid
> >> implementing offline functions in future.
> > 
> > the more I think about it, the more I like this approach. The core
> > change would be to just allow SMB_VFS_GET_DOS_ATTRIBUTES() to return
> > FILE_ATTRIBUTE_OFFLINE. VFS modules that support this would then
> > simply skip an implementation of SMB_VFS_IS_OFFLINE. Hm....
> 
> I actually like the original idea. From previous discussions on this
> list, we don't care about compatibility in VFS modules (except for
> incrementing the version number). There has to be a compelling reason to
> have a separate offline manipulation code (for example the
> create_file_fn VFS is complex, and most of the time you want to
> customize just the UNIX open, not all the bookkeeping around it, so we
> have both create_file_fn and open_fn).

this reasoning is exactly what lead me to wiping the slate clean and
removing the offline function: there's nothing that can't also be done
from an enhanced SMB_VFS_{GET|SET}_DOS_ATTRIBUTES().

> Another remark is that "old_mode" seems odd - usually a "set" function
> is just given the new desired state, and maybe it returns the old
> state.

That was in attempt to mimic the current behaviour where
file_set_dosmode() doesn the check, comparing old_dosmode against the
new_dosmode and then only calling SMB_VFS_SET_OFFLINE() in case the
the offline flag was not set in old_dosmode but is set in new_dosmode.

> There's one module that needs the previous offline state, but it can
> handle it by itself.

Good point! This way wouldn't have to always call dosmode() in
file_set_dosmode() and only do this in the single module that needs
it. Thanks!

> Other than that:
> 
> >  static NTSTATUS vfswrap_get_dos_attributes(struct vfs_handle_struct *handle,
> >  					   struct smb_filename *smb_fname,
> >  					   uint32_t *dosmode)
> >  {
> > +	bool offline;
> > +
> > +	offline = vfswrap_is_offline(handle, smb_fname, &smb_fname->st);
> > +	if (offline) {
> > +		*dosmode |= FILE_ATTRIBUTE_OFFLINE;
> > +	}
> > +
> >  	return get_ea_dos_attribute(handle->conn, smb_fname, dosmode);
> >  }
> 
> From a superficial check, I didn't find a reason for keeping the input
> bits and just OR'ing them with what we find. IMHO it would be cleaner if
> we reset dosmode to zero here and not rely on caller doing it for
> us.

we can't initialize it here, other modules in the stack might have
been called before us and I don't want to impose an order how the
individual module must implement the function wrt the order of calling
the next function and or'ing its own result.

> > +static NTSTATUS offline_get_dos_attributes(struct vfs_handle_struct *handle,
> > +					   struct smb_filename *smb_fname,
> > +					   uint32_t *dosmode)
> >  {
> > -	return true;
> > +	*dosmode |= FILE_ATTRIBUTE_OFFLINE;
> > +	return get_ea_dos_attribute(handle->conn, smb_fname, dosmode);
> > +}
> > +
> 
> If you agree to last comment, the the order here needs to be reversed.
> 
> Also, I think it's more cannonical to call the lower layer
> (VFS_NEXT_...) instead of jumping to the bottom layer.

oh, good catch. It ended up this way because previously both
vfs_offline and vfs_tsmsm implemented the offline functions as opaque
functions, not calling the next VFS module.

Thanks!
-slow