Re: [Xen-devel] [PATCH v4 2/2] xen/vm-events: Move parts of monitor_domctl code to common-side.

[Corneliu ZUZU]

On 2/16/2016 6:02 PM, Tamas K Lengyel wrote:


    @@ -143,77 +72,75 @@ int monitor_domctl(struct domain *d, struct
    xen_domctl_monitor_op *mop)

         case XEN_DOMCTL_MONITOR_EVENT_MOV_TO_MSR:
         {


So since we will now have two separate booleans, requested_status and 
old_status and then manually verify they are opposite..

    -        bool_t status = ad->monitor.mov_to_msr_enabled;
    +        bool_t old_status = ad->monitor.mov_to_msr_enabled;


...here we should set the field to requested_status, not !old_status. 
While they are technically equivalent, the code would read better to 
other way around.


    -        ad->monitor.mov_to_msr_enabled = !status;
    +        ad->monitor.mov_to_msr_enabled = !old_status; 

             domain_unpause(d);
             break;
         }

         case XEN_DOMCTL_MONITOR_EVENT_SINGLESTEP:
         {
    -        bool_t status = ad->monitor.singlestep_enabled;
    +        bool_t old_status = ad->monitor.singlestep_enabled;

    -        rc = status_check(mop, status);
    -        if ( rc )
    -            return rc;
    +        if ( unlikely(old_status == requested_status) )
    +            return -EEXIST;

             domain_pause(d);


Here as well..

    -        ad->monitor.singlestep_enabled = !status;
    +        ad->monitor.singlestep_enabled = !old_status;
             domain_unpause(d);
             break;
         }

         case XEN_DOMCTL_MONITOR_EVENT_SOFTWARE_BREAKPOINT:
         {
    -        bool_t status = ad->monitor.software_breakpoint_enabled;
    +        bool_t old_status = ad->monitor.software_breakpoint_enabled;

    -        rc = status_check(mop, status);
    -        if ( rc )
    -            return rc;
    +        if ( unlikely(old_status == requested_status) )
    +            return -EEXIST;

             domain_pause(d);


..and here..

    -        ad->monitor.software_breakpoint_enabled = !status;
    +        ad->monitor.software_breakpoint_enabled = !old_status;
             domain_unpause(d);
             break;
         }

         case XEN_DOMCTL_MONITOR_EVENT_GUEST_REQUEST:
         {
    -        bool_t status = ad->monitor.guest_request_enabled;
    +        bool_t old_status = ad->monitor.guest_request_enabled;

    -        rc = status_check(mop, status);
    -        if ( rc )
    -            return rc;
    +        if ( unlikely(old_status == requested_status) )
    +            return -EEXIST;

             domain_pause(d);
             ad->monitor.guest_request_sync = mop->u.guest_request.sync;


..and here.

    -        ad->monitor.guest_request_enabled = !status;
    +        ad->monitor.guest_request_enabled = !old_status;
             domain_unpause(d);
             break;
         }


Otherwise the patch looks good.

Thanks,
Tamas


Oh, right, that would look better. Shall I send a v5 then with that 
change? (and if yes I guess it won't hurt if I also include the 
left-shift sanity checks I mentioned I should have added (?))

Thanks,
Corneliu.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel