Re: [Xen-devel] [PATCH v5] x86/mm: Clean up p2m_finish_type_change return value

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 29.03.19 at 13:50, <aisaila@xxxxxxxxxxxxxxx> wrote:
> @@ -1174,9 +1174,9 @@ static int finish_type_change(struct p2m_domain *p2m,
>          /*
>           * ept->recalc could return 0/1/-ENOMEM. pt->recalc could return
>           * 0/-ENOMEM/-ENOENT, -ENOENT isn't an error as we are looping
> -         * gfn here.
> +         * gfn here. If rc is 1 we need to have it 0 for success.
>           */
> -        if ( rc == -ENOENT )
> +        if ( rc == -ENOENT || rc == 1 )

I guess you mean "rc > 0" here? This and the comment are of course
easy enough to adjust while committing, and with the adjustments
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> @@ -1213,19 +1213,13 @@ int p2m_finish_type_change(struct domain *d,
>              if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
>              {
>                  struct p2m_domain *altp2m = d->arch.altp2m_p2m[i];
> -                int rc1;
>  
>                  p2m_lock(altp2m);
> -                rc1 = finish_type_change(altp2m, first_gfn, max_nr);
> +                rc = finish_type_change(altp2m, first_gfn, max_nr);
>                  p2m_unlock(altp2m);
>  
> -                if ( rc1 < 0 )
> -                {
> -                    rc = rc1;
> +                if ( rc < 0 )

Along the lines of an earlier comment - I think it is generally
better to not special case negative return values when positive
ones don't have special meaning. But I know the code base is
very inconsistent about this, so I don't really mind leaving it as
is.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel