[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v6 11/15] x86/smpboot: clone_mapping should have one exit path



On 24.04.2020 16:09, Hongyan Xia wrote:
> From: Wei Liu <wei.liu2@xxxxxxxxxx>

Like for patches 1 and 2 in this series, and as iirc mentioned already
long ago for those, "should" or alike are misleading here: It's not a
mistake that they don't, i.e. this is not a bug fix. You _want_ these
functions to have a single (main) exit path, for subsequent changes of
yours ending up easier.

> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -675,6 +675,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
> *rpt)
>      l3_pgentry_t *pl3e;
>      l2_pgentry_t *pl2e;
>      l1_pgentry_t *pl1e;
> +    int rc = -ENOMEM;

Would you mind starting out with 0 here, ...

> @@ -715,7 +716,10 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
> *rpt)
>              pl1e = l2e_to_l1e(*pl2e) + l1_table_offset(linear);
>              flags = l1e_get_flags(*pl1e);
>              if ( !(flags & _PAGE_PRESENT) )
> -                return 0;
> +            {
> +                rc = 0;
> +                goto out;
> +            }

... dropping assignment and braces here, and ...

> @@ -724,7 +728,7 @@ static int clone_mapping(const void *ptr, root_pgentry_t 
> *rpt)
>      {
>          pl3e = alloc_xen_pagetable();
>          if ( !pl3e )
> -            return -ENOMEM;
> +            goto out;

... setting rc to -ENOMEM ahead of the if() up from here?
This imo makes things then not only minimally shorter, but
also fights slightly better with the nevertheless still
existing (after this patch) separate early exit paths.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.