On Tue, Feb 16, 2016 at 10:48 AM, Corneliu ZUZU <cz...@bitdefender.com>
wrote:
> 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 (?))
>
Please do send another revision with these changes. As I understood Jan
prefers the sanity-checks to be added as a separate patch, so do that.
Thanks,
Tamas
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
