[PATCH] cifs: handle cifs_get_tcon() errors properly

[PATCH] cifs: handle cifs_get_tcon() errors properly

[Suresh Jayaraman]

cifs_get_tcon() could return any of the following errors:
-ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
otherwise.

Compile-tested only.

Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
---
 fs/cifs/connect.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..c3a2323 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2780,7 +2780,10 @@ try_mount_again:
 	if (IS_ERR(tcon)) {
 		rc = PTR_ERR(tcon);
 		tcon = NULL;
-		goto remote_path_check;
+		if (rc == -EREMOTEIO)
+			goto remote_path_check;
+		else
+			goto mount_fail_check;
 	}
 
 	/* do not care if following two calls succeed - informational */

Re: [PATCH] cifs: handle cifs_get_tcon() errors properly

[Jeff Layton]

On Mon, 15 Nov 2010 18:15:23 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> cifs_get_tcon() could return any of the following errors:
> -ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
> when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
> otherwise.
> 
> Compile-tested only.
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> ---
>  fs/cifs/connect.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..c3a2323 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2780,7 +2780,10 @@ try_mount_again:
>  	if (IS_ERR(tcon)) {
>  		rc = PTR_ERR(tcon);
>  		tcon = NULL;
> -		goto remote_path_check;
> +		if (rc == -EREMOTEIO)
> +			goto remote_path_check;
> +		else
> +			goto mount_fail_check;
>  	}
>  
>  	/* do not care if following two calls succeed - informational */

Don't you mean "EREMOTE" here? I've always interpreted "EREMOTEIO" to
mean that the server had the equivalent of an I/O error, whereas
"EREMOTE" means "Object is remote".

I don't see how this patch materially changes anything. Pseudocode,
assuming that rc == -EREMOTE:

remote_path_check:
        /* check if a whole path (including prepath) is not remote */
        if (!rc && cifs_sb->prepathlen && tcon) {
		/* stuff we won't hit because rc is non-zero */
	}

	if (rc == -EREMOTE) {
		/* handle a DFS referral */
	}

        if (rc)
                goto mount_fail_check;

I'll grant that cifs_mount is still a mess, but I'm not sure this
really improves anything.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: handle cifs_get_tcon() errors properly

[Suresh Jayaraman]

On 11/15/2010 06:55 PM, Jeff Layton wrote:
> On Mon, 15 Nov 2010 18:15:23 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> cifs_get_tcon() could return any of the following errors:
>> -ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
>> when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
>> otherwise.
>>
>> Compile-tested only.
>>
>> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>> ---
>>  fs/cifs/connect.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 251a17c..c3a2323 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -2780,7 +2780,10 @@ try_mount_again:
>>  	if (IS_ERR(tcon)) {
>>  		rc = PTR_ERR(tcon);
>>  		tcon = NULL;
>> -		goto remote_path_check;
>> +		if (rc == -EREMOTEIO)
>> +			goto remote_path_check;
>> +		else
>> +			goto mount_fail_check;
>>  	}
>>  
>>  	/* do not care if following two calls succeed - informational */
> 
> Don't you mean "EREMOTE" here? I've always interpreted "EREMOTEIO" to

Doh, yes. 

> mean that the server had the equivalent of an I/O error, whereas
> "EREMOTE" means "Object is remote".
> 
> I don't see how this patch materially changes anything. Pseudocode,
> assuming that rc == -EREMOTE:

I agree that this is not an improvement (more of a cleanup), but I think
it improves readability (given that the single function runs for more than 
hundred lines), and skips a couple of unneeded checks.

Here's a fixed version (but this patch can be ignored if we feel so):


cifs_get_tcon() could return any of the following errors:
-ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
otherwise.

Compile-tested only.

Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
--- 
 fs/cifs/connect.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 251a17c..6dbc145 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2780,7 +2780,10 @@ try_mount_again:
 	if (IS_ERR(tcon)) {
 		rc = PTR_ERR(tcon);
 		tcon = NULL;
-		goto remote_path_check;
+		if (rc == -EREMOTE)
+			goto remote_path_check;
+		else
+			goto mount_fail_check;
 	}
 
 	/* do not care if following two calls succeed - informational */

Re: [PATCH] cifs: handle cifs_get_tcon() errors properly

[Jeff Layton]

On Mon, 15 Nov 2010 20:24:41 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:

> On 11/15/2010 06:55 PM, Jeff Layton wrote:
> > On Mon, 15 Nov 2010 18:15:23 +0530
> > Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> > 
> >> cifs_get_tcon() could return any of the following errors:
> >> -ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
> >> when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
> >> otherwise.
> >>
> >> Compile-tested only.
> >>
> >> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> >> ---
> >>  fs/cifs/connect.c |    5 ++++-
> >>  1 files changed, 4 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> >> index 251a17c..c3a2323 100644
> >> --- a/fs/cifs/connect.c
> >> +++ b/fs/cifs/connect.c
> >> @@ -2780,7 +2780,10 @@ try_mount_again:
> >>  	if (IS_ERR(tcon)) {
> >>  		rc = PTR_ERR(tcon);
> >>  		tcon = NULL;
> >> -		goto remote_path_check;
> >> +		if (rc == -EREMOTEIO)
> >> +			goto remote_path_check;
> >> +		else
> >> +			goto mount_fail_check;
> >>  	}
> >>  
> >>  	/* do not care if following two calls succeed - informational */
> > 
> > Don't you mean "EREMOTE" here? I've always interpreted "EREMOTEIO" to
> 
> Doh, yes. 
> 
> > mean that the server had the equivalent of an I/O error, whereas
> > "EREMOTE" means "Object is remote".
> > 
> > I don't see how this patch materially changes anything. Pseudocode,
> > assuming that rc == -EREMOTE:
> 
> I agree that this is not an improvement (more of a cleanup), but I think
> it improves readability (given that the single function runs for more than 
> hundred lines), and skips a couple of unneeded checks.
> 
> Here's a fixed version (but this patch can be ignored if we feel so):
> 
> 
> cifs_get_tcon() could return any of the following errors:
> -ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
> when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
> otherwise.
> 
> Compile-tested only.
> 
> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
> --- 
>  fs/cifs/connect.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 251a17c..6dbc145 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -2780,7 +2780,10 @@ try_mount_again:
>  	if (IS_ERR(tcon)) {
>  		rc = PTR_ERR(tcon);
>  		tcon = NULL;
> -		goto remote_path_check;
> +		if (rc == -EREMOTE)
> +			goto remote_path_check;
> +		else
> +			goto mount_fail_check;
>  	}
>  
>  	/* do not care if following two calls succeed - informational */

I just don't the point here. cifs_mount is quite messy, but I don't
think this really does much to improve its readability.

-- 
Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Re: [PATCH] cifs: handle cifs_get_tcon() errors properly

[Suresh Jayaraman]

On 11/15/2010 08:58 PM, Jeff Layton wrote:
> On Mon, 15 Nov 2010 20:24:41 +0530
> Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> wrote:
> 
>> I agree that this is not an improvement (more of a cleanup), but I think
>> it improves readability (given that the single function runs for more than 
>> hundred lines), and skips a couple of unneeded checks.
>>
>> Here's a fixed version (but this patch can be ignored if we feel so):
>>
>>
>> cifs_get_tcon() could return any of the following errors:
>> -ENOMEM/-ENODEV/EREMOTEIO. We should follow the DFS referral code path only
>> when we get -EREMOTEIO (STATUS_PATH_NOT_COVERED) from the server and not
>> otherwise.
>>
>> Compile-tested only.
>>
>> Signed-off-by: Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org>
>> --- 
>>  fs/cifs/connect.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
>> index 251a17c..6dbc145 100644
>> --- a/fs/cifs/connect.c
>> +++ b/fs/cifs/connect.c
>> @@ -2780,7 +2780,10 @@ try_mount_again:
>>  	if (IS_ERR(tcon)) {
>>  		rc = PTR_ERR(tcon);
>>  		tcon = NULL;
>> -		goto remote_path_check;
>> +		if (rc == -EREMOTE)
>> +			goto remote_path_check;
>> +		else
>> +			goto mount_fail_check;
>>  	}
>>  
>>  	/* do not care if following two calls succeed - informational */
> 
> I just don't the point here. cifs_mount is quite messy, but I don't
> think this really does much to improve its readability.
> 

Ok, let's drop this patch..


-- 
Suresh Jayaraman