http://git.samba.org/?p=tprouty/samba.git; a=shortlog; h=dos_attr_work

[Tim Prouty]

Hi James,

Thanks for the feedback!

On Nov 10, 2008, at 5:18 PM, James Peach wrote:

> 1. You should #ifdef some of the UF_* constants, because lots of
> systems don't have more than the minimal set or use different names.
> The names you have don't seem to be the standard BSD names.

We have extended the standard bsd inode and stat st_flags field to add  
support for getting/setting CIFS attributes, which is why the names  
are not standard BSD names.  They are #ifdef'd by the configure check  
(which is justified below), so that systems using a different system  
for getting/setting attributes are not affected.

> 2. When you are getting the flags, you should also check the SF_*
> equivalents, eg, SF_IMMUTABLE.

I'm only interested in the special windows specific attributes when  
getting getting the flags (UF_DOS_*) of which there are no SF_*  
equivalents.  If the SF_IMMUTABLE flag is set, the VOP_SETATTR  
implementation of the file system will decide what to do when setting  
the flags with chflags.  The standard behavior is to return EPERM,  
which is translated to NT_STATUS_ACCESS_DENIED.  This seems  
reasonable, so I'm not sure what a user-space check for SF_IMMUTABLE  
would provide.

> 3. The DOS archive bit is actually the inverse of the BSD archive bit.
> Although the standard BSD mit is UF_NODUMP, so maybe UF_DOS_ARCHIVE is
> different?

Yes, this implementation of storing DOS attributes adds new flags in  
order to avoid overloading the meaning of existing unix flags.

> 4. My patch also checks UF_IMMUTABLE in can_access_file() ... not sure
> whether you would still need that.

For the same reason of not checking for SF_IMMUTABLE, I don't think a  
check for UF_IMMUTABLE is necessary in this case.

> 5. You don't need the configure check at all. SMB_VFS_CHFLAGS takes
> care of portably setting the flags and there is already a check for
> the st_flags field. Just wrap the usages of UD_DOS_* in #ifdefs.

I'm trying to write this in such a way that other people could add  
support for storing dos attributes in the st_flags for other operating  
systems.  With this in mind it seems cleaner to make this a configure  
check, and use the result for the #ifdef checks.  This way the ability  
to store dos attributes in st_flags is either on or off.

> 6. The change to proto.h seems spurious?

I added it for another patch I'm working on, that uses the helper  
function.  I agree it is spurious at this point, and I'll make it part  
of the other patch that utilizes it, rather than including it as part  
of this patch.

-Tim