[patch] cifs: return more accurate errno

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] cifs: return more accurate errno

Dan Carpenter
Smatch compains that we don't use the return value from get_dfs_path().

In the original code if get_dfs_path() fails we return ERR_PTR(-ENOENT),
but with this patch we can return errno from get_dfs_path() directly.

Signed-off-by: Dan Carpenter <[hidden email]>
---
Compile tested only.  Sorry.  :/

diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
index 78e4d2a..fa1f74a 100644
--- a/fs/cifs/cifs_dfs_ref.c
+++ b/fs/cifs/cifs_dfs_ref.c
@@ -345,6 +345,8 @@ cifs_dfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
  rc = get_dfs_path(xid, ses , full_path + 1, cifs_sb->local_nls,
  &num_referrals, &referrals,
  cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
+ if (rc < 0)
+ goto out_err;
 
  for (i = 0; i < num_referrals; i++) {
  int len;
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [patch] cifs: return more accurate errno

Jiri Kosina-2
On Fri, 7 May 2010, Dan Carpenter wrote:

> Smatch compains that we don't use the return value from get_dfs_path().
>
> In the original code if get_dfs_path() fails we return ERR_PTR(-ENOENT),
> but with this patch we can return errno from get_dfs_path() directly.
>
> Signed-off-by: Dan Carpenter <[hidden email]>
> ---
> Compile tested only.  Sorry.  :/
>
> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
> index 78e4d2a..fa1f74a 100644
> --- a/fs/cifs/cifs_dfs_ref.c
> +++ b/fs/cifs/cifs_dfs_ref.c
> @@ -345,6 +345,8 @@ cifs_dfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
>   rc = get_dfs_path(xid, ses , full_path + 1, cifs_sb->local_nls,
>   &num_referrals, &referrals,
>   cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
> + if (rc < 0)
> + goto out_err;
>  
>   for (i = 0; i < num_referrals; i++) {
>   int len;

Has this ever been picked up? Doesn't seem to be present in linux-next.

Thanks,

--
Jiri Kosina
SUSE Labs, Novell Inc.
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [patch] cifs: return more accurate errno

Steve French-2
I will check.

On Tue, May 18, 2010 at 7:06 AM, Jiri Kosina <[hidden email]> wrote:

> On Fri, 7 May 2010, Dan Carpenter wrote:
>
>> Smatch compains that we don't use the return value from get_dfs_path().
>>
>> In the original code if get_dfs_path() fails we return ERR_PTR(-ENOENT),
>> but with this patch we can return errno from get_dfs_path() directly.
>>
>> Signed-off-by: Dan Carpenter <[hidden email]>
>> ---
>> Compile tested only.  Sorry.  :/
>>
>> diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c
>> index 78e4d2a..fa1f74a 100644
>> --- a/fs/cifs/cifs_dfs_ref.c
>> +++ b/fs/cifs/cifs_dfs_ref.c
>> @@ -345,6 +345,8 @@ cifs_dfs_follow_mountpoint(struct dentry *dentry, struct nameidata *nd)
>>       rc = get_dfs_path(xid, ses , full_path + 1, cifs_sb->local_nls,
>>               &num_referrals, &referrals,
>>               cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MAP_SPECIAL_CHR);
>> +     if (rc < 0)
>> +             goto out_err;
>>
>>       for (i = 0; i < num_referrals; i++) {
>>               int len;
>
> Has this ever been picked up? Doesn't seem to be present in linux-next.
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs, Novell Inc.
>



--
Thanks,

Steve
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [patch] cifs: return more accurate errno

Steve French-2
On Tue, May 18, 2010 at 12:18 PM, Steve French <[hidden email]> wrote:

> I will check.
>
> On Tue, May 18, 2010 at 7:06 AM, Jiri Kosina <[hidden email]> wrote:
>> On Fri, 7 May 2010, Dan Carpenter wrote:
>>
>>> Smatch compains that we don't use the return value from get_dfs_path().
>>>
>>> In the original code if get_dfs_path() fails we return ERR_PTR(-ENOENT),
>>> but with this patch we can return errno from get_dfs_path() directly.
>>>
>>> Signed-off-by: Dan Carpenter <[hidden email]>

ENOENT does seem like a safe return code if a DFS referral can not be
followed, but if the intent was to remap all error cases here to ENOENT
it would be easier to follow with something similar to your code e.g.

+       if (rc < 0) {
+               rc = -ENOENT;
+               goto out_err;
+      }

Igor,
Did you intend to always return ENOENT for DFS when failures looking up
DFS referrals (probably most likely to be either error coming back from
 tree connect to IPC$ or error from the call to GetDFSRefer)?

--
Thanks,

Steve

)
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [patch] cifs: return more accurate errno

Suresh Jayaraman-2
On 05/19/2010 11:02 AM, Steve French wrote:

> On Tue, May 18, 2010 at 12:18 PM, Steve French <[hidden email]> wrote:
>> I will check.
>>
>> On Tue, May 18, 2010 at 7:06 AM, Jiri Kosina <[hidden email]> wrote:
>>> On Fri, 7 May 2010, Dan Carpenter wrote:
>>>
>>>> Smatch compains that we don't use the return value from get_dfs_path().
>>>>
>>>> In the original code if get_dfs_path() fails we return ERR_PTR(-ENOENT),
>>>> but with this patch we can return errno from get_dfs_path() directly.
>>>>
>>>> Signed-off-by: Dan Carpenter <[hidden email]>
>
> ENOENT does seem like a safe return code if a DFS referral can not be
> followed, but if the intent was to remap all error cases here to ENOENT
> it would be easier to follow with something similar to your code e.g.
>
> +       if (rc < 0) {
> +               rc = -ENOENT;
> +               goto out_err;
> +      }
>
> Igor,
> Did you intend to always return ENOENT for DFS when failures looking up

I don't think that was the intent. When called from cifs_mount, we seem
to be passing down the error code without remapping.

The GET_DFS_REFERRAL call could return -EREMOTE for example and
get_dfs_path() could return other valid errors say -ENODEV etc. I think
the original error code should be propagated back and the patch looks
correct to me.


> DFS referrals (probably most likely to be either error coming back from
>  tree connect to IPC$ or error from the call to GetDFSRefer)?
>


Thanks,

--
Suresh Jayaraman
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client
Reply | Threaded
Open this post in threaded view
|

Re: [patch] cifs: return more accurate errno

Jeff Layton-4
On Wed, 19 May 2010 11:23:13 +0530
Suresh Jayaraman <[hidden email]> wrote:

> On 05/19/2010 11:02 AM, Steve French wrote:
> > On Tue, May 18, 2010 at 12:18 PM, Steve French <[hidden email]> wrote:
> >> I will check.
> >>
> >> On Tue, May 18, 2010 at 7:06 AM, Jiri Kosina <[hidden email]> wrote:
> >>> On Fri, 7 May 2010, Dan Carpenter wrote:
> >>>
> >>>> Smatch compains that we don't use the return value from get_dfs_path().
> >>>>
> >>>> In the original code if get_dfs_path() fails we return ERR_PTR(-ENOENT),
> >>>> but with this patch we can return errno from get_dfs_path() directly.
> >>>>
> >>>> Signed-off-by: Dan Carpenter <[hidden email]>
> >
> > ENOENT does seem like a safe return code if a DFS referral can not be
> > followed, but if the intent was to remap all error cases here to ENOENT
> > it would be easier to follow with something similar to your code e.g.
> >
> > +       if (rc < 0) {
> > +               rc = -ENOENT;
> > +               goto out_err;
> > +      }
> >
> > Igor,
> > Did you intend to always return ENOENT for DFS when failures looking up
>
> I don't think that was the intent. When called from cifs_mount, we seem
> to be passing down the error code without remapping.
>
> The GET_DFS_REFERRAL call could return -EREMOTE for example and
> get_dfs_path() could return other valid errors say -ENODEV etc. I think
> the original error code should be propagated back and the patch looks
> correct to me.
>
>

I think the original patch is correct too, or at least preserves
existing behavior better.

That said, there's no real "bug" here at the moment. The existing code
seems to always set num_referrals to 0 when there's an error, so the
for loop is always skipped. That's probably not something we ought to
depend on however, so I think Dan's patch is good.

Reviewed-by: Jeff Layton <[hidden email]>
_______________________________________________
linux-cifs-client mailing list
[hidden email]
https://lists.samba.org/mailman/listinfo/linux-cifs-client