SMB_VFS_GET_DOS_ATTRIBUTES vs SMB_VFS_IS_OFFLINE

Alexander Bokovoy ab at samba.org
Sun Sep 11 19:09:01 UTC 2016


On Sun, 11 Sep 2016, 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....
> > 
> > Cheerio!
> > -slow
> > 
> 
> 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
We do care about compatibility as we don't want to break minor releases.
Breaking API between major releases is considerable but not for minor ones. Note
that this explicitly a no go for deleting API as Ralph's WIP patch
suggests. If we want to migrate all modules to new API, we can do so
in this case without breaking the old ABI.

> 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).
Offline support was added to allow plug in storage backends with
different cost of access. This includes tapes and other slower but more
reliable storage. It is expected to take some time to initialize such
backends and do more complex operations on them.

 
> 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.
> There's one module that needs the previous offline state, but it can
> handle it by itself.
> 
> 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.
> 
> > +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.
> 
> Same goes for tsmsm module.
> 
> Thanks,
> Uri.

-- 
/ Alexander Bokovoy



More information about the samba-technical mailing list