Re: [PATCH] domctl: cleanup

[Andrew Cooper <andrew.cooper3@citrix.com>]

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:
>      {
>