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

[Corneliu ZUZU <czuzu@bitdefender.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3190 bytes --]

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.

[-- Attachment #1.2: Type: text/html, Size: 7049 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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