Re: [PATCH v20210701 12/40] tools: unify type checking for data pfns in migration stream

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/07/2021 10:56, Olaf Hering wrote:
> Introduce a helper which decides if a given pfn in the migration
> stream is backed by memory.
>
> This specifically deals with type XEN_DOMCTL_PFINFO_XALLOC, which was
> a synthetic toolstack-only type used in Xen 4.2 to 4.5. It indicated a
> dirty page on the sending side for which no data will be send in the
> initial iteration.
>
> No change in behavior intended.
>
> Signed-off-by: Olaf Hering <olaf@xxxxxxxxx>
> ---
>  tools/libs/saverestore/common.h  | 17 +++++++++++++++++
>  tools/libs/saverestore/restore.c |  5 ++---
>  2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libs/saverestore/common.h b/tools/libs/saverestore/common.h
> index 07c506360c..fa242e808d 100644
> --- a/tools/libs/saverestore/common.h
> +++ b/tools/libs/saverestore/common.h
> @@ -500,6 +500,23 @@ static inline bool sr_is_known_page_type(xen_pfn_t type)
>      return ret;
>  }
>  
> +static inline bool page_type_to_populate(uint32_t type)
> +{
> +    bool ret;
> +
> +    switch (type)
> +    {

Same style comments as before.

> +    case XEN_DOMCTL_PFINFO_XTAB:
> +    case XEN_DOMCTL_PFINFO_BROKEN:
> +        ret = false;
> +        break;
> +    case XEN_DOMCTL_PFINFO_XALLOC:
> +    default:
> +        ret = true;
> +        break;

I know you're replacing the logic as-was, but in hindsight, I'm not sure
it was great to begin with.  It defaults the unallocated types to being
considered populated, which isn't a clever idea.

Anyone adding a new page type is going to have to audit/edit each of
these helpers.  I think it would be better to write all the true cases
explicitly.

> +    }
> +    return ret;
> +}
>  #endif
>  /*
>   * Local variables:
> diff --git a/tools/libs/saverestore/restore.c 
> b/tools/libs/saverestore/restore.c
> index 324b9050e2..477b7527a1 100644
> --- a/tools/libs/saverestore/restore.c
> +++ b/tools/libs/saverestore/restore.c
> @@ -152,9 +152,8 @@ int populate_pfns(struct xc_sr_context *ctx, unsigned int 
> count,
>  
>      for ( i = 0; i < count; ++i )
>      {
> -        if ( (!types || (types &&
> -                         (types[i] != XEN_DOMCTL_PFINFO_XTAB &&
> -                          types[i] != XEN_DOMCTL_PFINFO_BROKEN))) &&
> +        if ( (!types ||
> +              (types && page_type_to_populate(types[i]) == true)) &&

I'm surprised not to have seen a compiler or static analysis complaint
about this.

!A || (A && B) is redundant, and simplifies to !A || B.

Clearly need to blame whichever numpty wrote this code to begin with.

~Andrew