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

[Steve French]

Does it matter, if all of the callers of this function always call it
with one of the flags specified?

The "default" open mode is read/write (that was the only one smbfs used)

On Wed, May 28, 2008 at 8:48 PM, 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
>
>
>
>



-- 
Thanks,

Steve