From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] domctl: cleanup
Date: Tue, 3 Mar 2015 20:12:26 +0000
Message-ID: <54F615AA.7040009@citrix.com>
References: <54F5D5F50200007800065BCD@mail.emea.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
Received: from mail6.bemta14.messagelabs.com ([193.109.254.103])
	by lists.xen.org with esmtp (Exim 4.72)
	(envelope-from <Andrew.Cooper3@citrix.com>) id 1YStBK-0000vT-7m
	for xen-devel@lists.xenproject.org; Tue, 03 Mar 2015 20:12:42 +0000
In-Reply-To: <54F5D5F50200007800065BCD@mail.emea.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, xen-devel <xen-devel@lists.xenproject.org>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Keir Fraser <keir@xen.org>, Tim Deegan <tim@xen.org>
List-Id: xen-devel@lists.xenproject.org

On 03/03/15 14:40, Jan Beulich wrote:
> - drop redundant "ret = 0" statements
> - drop unnecessary braces
> - eliminate a few single use local variables
> - move break statements inside case-specific braced scopes
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

One bug (one ret = 0 was not redundant) and one suggestion for futher
cleanup.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -831,15 +825,13 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>                  ret = cpumask_to_xenctl_bitmap(&vcpuaff->cpumap_soft,
>                                                 v->cpu_soft_affinity);
>          }
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_scheduler_op:
> -    {
>          ret = sched_adjust(d, &op->u.scheduler_op);
>          copyback = 1;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_getdomaininfo:
>      { 

For a cleanup patch, you might as well nuke the trailing whitespace,
which includes the a space after this brace....

> @@ -870,8 +859,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>      getdomaininfo_out:
>          rcu_read_unlock(&domlist_read_lock);
>          d = NULL;
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_getvcpucontext:
>      { 

... and here ...

> @@ -919,8 +908,8 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>  
>      getvcpucontext_out:
>          xfree(c.nat);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_getvcpuinfo:
>      { 

... and here.

> @@ -961,21 +947,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>           * the meantime, while tot > max, all new allocations are disallowed.
>           */
>          d->max_pages = new_max;
> -        ret = 0;
>          spin_unlock(&d->page_alloc_lock);
> +        break;
>      }
> -    break;
>  
>      case XEN_DOMCTL_setdomainhandle:
> -    {
>          memcpy(d->handle, op->u.setdomainhandle.handle,
>                 sizeof(xen_domain_handle_t));
> -        ret = 0;
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_setdebugging:
> -    {
>          ret = -EINVAL;
>          if ( d == current->domain ) /* no domain_pause() */
>              break;
> @@ -983,9 +964,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          domain_pause(d);
>          d->debugger_attached = !!op->u.setdebugging.enable;
>          domain_unpause(d); /* causes guest to latch new status */
> -        ret = 0;

This ret is not redundant.  (Observe the unconditional ret = -EINVAL in
the previous hunk).

XEN_DOMCTL_setdebugging can probably be rearranged to a single if()/else
in the same way as XEN_DOMCTL_set_access_required below to make it
slightly shorter.

~Andrew

> @@ -1131,41 +1103,28 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
>          break;
>  
>      case XEN_DOMCTL_disable_migrate:
> -    {
>          d->disable_migrate = op->u.disable_migrate.disable;
> -    }
> -    break;
> +        break;
>  
>  #ifdef HAS_MEM_ACCESS
>      case XEN_DOMCTL_set_access_required:
> -    {
> -        struct p2m_domain* p2m;
> -
> -        ret = -EPERM;
>          if ( current->domain == d )
> -            break;
> -
> -        ret = 0;
> -        p2m = p2m_get_hostp2m(d);
> -        p2m->access_required = op->u.access_required.access_required;
> -    }
> -    break;
> +            ret = -EPERM;
> +        else
> +            p2m_get_hostp2m(d)->access_required =
> +                op->u.access_required.access_required;
> +        break;
>  #endif
>  
>      case XEN_DOMCTL_set_virq_handler:
> -    {
> -        uint32_t virq = op->u.set_virq_handler.virq;
> -        ret = set_global_virq_handler(d, virq);
> -    }
> -    break;
> +        ret = set_global_virq_handler(d, op->u.set_virq_handler.virq);
> +        break;
>  
>      case XEN_DOMCTL_set_max_evtchn:
> -    {
>          d->max_evtchn_port = min_t(unsigned int,
>                                     op->u.set_max_evtchn.max_port,
>                                     INT_MAX);
> -    }
> -    break;
> +        break;
>  
>      case XEN_DOMCTL_setvnumainfo:
>      {
>