Issue with changes to VFS_STAT and VFS_LSTAT.
tprouty at samba.org
Mon Jun 29 11:51:19 MDT 2009
Jeremy, thank you for bringing this up! It was an issue I was
planning on addressing at some point during this conversion, but
there's no reason not to just address it now. My responses are inline
On Jun 26, 2009, at 3:47 PM, Jeremy Allison wrote:
> This pushes the 'struct smb_filename' into visibility at the
> POSIX VFS layer, which as a shorthand to keep the stat struct
> tied to the filename is fine.
> However, in the default stat() implementation it now calls :
> get_full_smb_filename() which concatinates the base name + stream
> name (if one exists). Which means potentially the full stream
> names are visible to an underlying POSIX system than cannot
> cope with them.
This is actually true for vfswrap_open() as well, which has a comment
that is basically asking the same question.
> Is this desired ? The logic in the upper level smbd should
> ensure that basic POSIX stat is containing a stream name is never
> called if the underlying filesystem can't support them.
My reason for adding the call to get_full_smb_filename() in
vfs_default.c was to maintain the existing behavior of passing the
full name to the posix call if no modules implementing streams first
handled the call. This would be the case if smbd were run without
specifying any streams modules.
Previously there was no correct way at the vfs layer to differentiate
between a filename containing a colon (posix extensions, demangled
name) and a stream as passed over the wire by the windows client. Now
that there is more detailed information about the filename at the
posix level, I fully agree that this is a good time to diverge from
> My feeling is in the "default" module case which remember is
> the last stop before hitting a *pure* "POSIX" filesystem we should
> do an SMB_ASSERT() that smb_fname->stream_name == NULL, as I can't
> see any case where the "default" POSIX filesystem can do
> anything but error out here. It actually is exposing a logic error
> in the upper level logic IMHO. A simple patch for this follows.
In the case of smbd running with no streams modules, I believe your
patch will cause an assert to be fired for every smb_filename-based
vfs op. What do you think about returning an error and setting ERRNO
to something like ENOENT if stream != NULL? We could even go one step
further and allow the vfs_default posix wrappers to allow opening
the ::$DEFAULT stream by checking is_ntfs_default_stream_smb_fname()
and using the base_name if it returns true.
I'm happy to make these changes in my next round of the migration.
Also, thank you for following all of these changes! I'm touching a
lot of code, and it's good to know you're keeping an eye on them :).
More information about the samba-technical