Re: [patch] cifs: return more accurate errno

On Wed, 19 May 2010 11:23:13 +0530
Suresh Jayaraman <[email protected]> wrote:

> On 05/19/2010 11:02 AM, Steve French wrote:
> > On Tue, May 18, 2010 at 12:18 PM, Steve French <[email protected]> wrote:
> >> I will check.
> >>
> >> On Tue, May 18, 2010 at 7:06 AM, Jiri Kosina <[email protected]> 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 <[email protected]>
> > 
> > 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 <[email protected]>