[PATCH v1] x86/mm: Clean up p2m_finish_type_change return value

[PATCH v1] x86/mm: Clean up p2m_finish_type_change return value

[Alexandru Stefan ISAILA]

Changed the return value of 1 to 0 so now p2m_finish_type_change returns
0 for success or <0 for error.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
 xen/arch/x86/mm/p2m.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d14ce57dd5..ae6459fa53 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1178,6 +1178,7 @@ int p2m_finish_type_change(struct domain *d,
 
     if ( rc < 0 )
         goto out;
+    rc = 0;
 
 #ifdef CONFIG_HVM
     if ( unlikely(altp2m_active(d)) )
@@ -1188,19 +1189,14 @@ 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 )
                     goto out;
-                }
-
-                rc |= rc1;
+                rc = 0;
             }
     }
 #endif
-- 
2.17.1


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

Re: [PATCH v1] x86/mm: Clean up p2m_finish_type_change return value

[Jan Beulich]

>>> On 16.01.19 at 16:13, <aisaila@bitdefender.com> wrote:
> Changed the return value of 1 to 0 so now p2m_finish_type_change returns
> 0 for success or <0 for error.

This is a valid alternative return value model. Both have their merits.
Hence if you want to change from one to the other, you should give
a reason.

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1178,6 +1178,7 @@ int p2m_finish_type_change(struct domain *d,
>  
>      if ( rc < 0 )
>          goto out;
> +    rc = 0;

Instead of this and ...

> @@ -1188,19 +1189,14 @@ 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 )
>                      goto out;
> -                }
> -
> -                rc |= rc1;
> +                rc = 0;

... this, why don't you simply set "rc" to zero once ...

>              }
>      }
>  #endif

... below here (but ahead of the "out" label)?

Jan



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

Re: [PATCH v1] x86/mm: Clean up p2m_finish_type_change return value

[Alexandru Stefan ISAILA]

On 16.01.2019 17:39, Jan Beulich wrote:
>>>> On 16.01.19 at 16:13, <aisaila@bitdefender.com> wrote:
>> Changed the return value of 1 to 0 so now p2m_finish_type_change returns
>> 0 for success or <0 for error.
> 
> This is a valid alternative return value model. Both have their merits.
> Hence if you want to change from one to the other, you should give
> a reason.

This was done after a discussion with George 
https://lists.xenproject.org/archives/html/xen-devel/2019-01/msg00740.html

I will update the patch comment with this reason.
> 
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -1178,6 +1178,7 @@ int p2m_finish_type_change(struct domain *d,
>>   
>>       if ( rc < 0 )
>>           goto out;
>> +    rc = 0;
> 
> Instead of this and ...
> 
>> @@ -1188,19 +1189,14 @@ 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 )
>>                       goto out;
>> -                }
>> -
>> -                rc |= rc1;
>> +                rc = 0;
> 
> ... this, why don't you simply set "rc" to zero once ...
> 
>>               }
>>       }
>>   #endif
> 
> ... below here (but ahead of the "out" label)?

I will move the rc = 0 there in v2.

Thanks,
Alex

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