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

[Jan Beulich <jbeulich@xxxxxxxx>]

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