[linux-cifs-client] Should recently added helper function access_flags_to_smbopen_mode() also catch the error case ?

[Jeff Layton]

On Thu, 29 May 2008 03:48:29 +0200
Günter Kukkukk <linux at kukkukk.com> wrote:

> Some days ago a helper function was added to interpret
> access_mode into valid open modes:
> 
> +static int
> +access_flags_to_smbopen_mode(const int access_flags)
> +{
> +       int masked_flags = access_flags & (GENERIC_READ | GENERIC_WRITE);
> +
> +       if (masked_flags == GENERIC_READ)
> +               return SMBOPEN_READ;
> +       else if (masked_flags == GENERIC_WRITE)
> +               return SMBOPEN_WRITE;
> +
> +       /* just go for read/write */
> +       return SMBOPEN_READWRITE;
> +}
> +
> 
> In case of neither passing GENERIC_READ nor GENERIC_WRITE - which
> would be an internal error - the default code path 
> (returning SMBOPEN_READWRITE) would be taken.
> I think, we should at least send an error message to the log
> and pass back "the least destructive mode SMBOPEN_READ". Or
> should we flag an error condition?
> 
> My current replacement:
> 
> static int
> access_flags_to_smbopen_mode(const int access_flags)
> {
> 	int ret;
> 	int masked_flags = access_flags & (GENERIC_READ | GENERIC_WRITE);
> 
> 	switch (masked_flags) {
> 	case GENERIC_READ:
> 		ret = SMBOPEN_READ;
> 		break;
> 	case GENERIC_WRITE:
> 		ret = SMBOPEN_WRITE;
> 		break;
> 	case (GENERIC_READ | GENERIC_WRITE):
> 		ret = SMBOPEN_READWRITE;
> 		break;
> 	default:
> 		cERROR(1, ("Internal error: Invalid access_flags passed"));
> 		ret = SMBOPEN_READ; /* BB ?? */
> 	}
> 	return ret;
> }
> 
> Comments ?
> Cheers, Günter
> 
> 
> 

I think you're right...

The reason I didn't do that was because the existing behavior was to
simply pass in SMBOPEN_READWRITE. cifs_open calls cifs_convert_flags,
and that seems like it can return this if no O_ACCMODE flag is set:

        return (READ_CONTROL | FILE_WRITE_ATTRIBUTES |
FILE_READ_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA |
FILE_WRITE_DATA | FILE_READ_DATA);

...but open(2) specifies that you must set one of O_RDONLY, O_WRONLY,
or O_RDWR. So it's not clear to me that we'll ever fall past all of the
if conditions in cifs_convert_flags and return that. I suppose there is
a possibility that something in-kernel could end up calling cifs_open()
with none of those flags set, though. I didn't want to have things fail
if that happened, but maybe they should, or we should at least warn if
it does.

On a side note, it also looks like cifs_open falls back to calling
SMBLegacyOpen, but cifs_reopen_file does not. I think we probably need
to fix that...

-- 
Jeff Layton <jlayton at redhat.com>