On Tue, 26 Aug 2008, Jeff Dike wrote:

> On Tue, Aug 19, 2008 at 09:09:16AM +0300, Ilpo Järvinen wrote:
> > ...I'm not fully sure what's the intention here, ie., whether
> > the return belongs to a block with the assignment or not.
> 
> Yes, this is confused, although it just happens to compile to
> something sane.

Yeah, I found ~20 similar brace vs reindent cases, for most of them (like 
this), it's currently insignificant but definately is trap for 1) a reader 
of the code and 2) future modification.

> Your patch is fine, but I think the whole thing needs
> some more cleanup.  See the patch below.

Seems much better than my approach, this case was anyway among the most 
complex within those ~20 case what comes to figuring out correctness, 
after your cleanup the code flow much more obvious.

-- 
> The testing for the host's TLS support was somewhat confused - notably
> in the indentation of a return.
> 
> This cleans it up:
>       *supports_tls is only set to 0 once
>       any error besides EINVAL causes an immediate return, with
> *supports_tls equal to 0.
> 
> Index: linux-2.6.22/arch/um/os-Linux/sys-i386/tls.c
> ===================================================================
> --- linux-2.6.22.orig/arch/um/os-Linux/sys-i386/tls.c 2007-07-08 
> 19:32:17.000000000 -0400
> +++ linux-2.6.22/arch/um/os-Linux/sys-i386/tls.c      2008-08-26 
> 08:19:37.000000000 -0400
> @@ -15,6 +15,7 @@ void check_host_supports_tls(int *suppor
>       int val[] = {GDT_ENTRY_TLS_MIN_I386, GDT_ENTRY_TLS_MIN_X86_64};
>       int i;
>  
> +     *supports_tls = 0;
>       for (i = 0; i < ARRAY_SIZE(val); i++) {
>               user_desc_t info;
>               info.entry_number = val[i];
> @@ -23,14 +24,9 @@ void check_host_supports_tls(int *suppor
>                       *tls_min = val[i];
>                       *supports_tls = 1;
>                       return;
> -             } else {
> -                     if (errno == EINVAL)
> -                             continue;
> -                     else if (errno == ENOSYS)
> -                             *supports_tls = 0;
> -                             return;
>               }
> +             if (errno == EINVAL)
> +                     continue;
> +             else return;

They usually say that you might want to put the return on a different line 
unless you have something to hide :-).

>       }
> -
> -     *supports_tls = 0;
>  }
> 

-- 
 i.
-------------------------------------------------------------------------
This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
Build the coolest Linux based applications with Moblin SDK & win great prizes
Grand prize is a trip for two to an Open Source event anywhere in the world
http://moblin-contest.org/redirect.php?banner_id=100&url=/
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Reply via email to