On 2/8/2016 8:17 PM, Tamas K Lengyel wrote:


On Mon, Feb 8, 2016 at 10:49 AM, Corneliu ZUZU <cz...@bitdefender.com <mailto:cz...@bitdefender.com>> wrote:

    On 2/8/2016 7:15 PM, Andrew Cooper wrote:

        On 08/02/16 16:57, Corneliu ZUZU wrote:

            diff --git a/xen/include/asm-x86/hvm/event.h
            b/xen/include/asm-x86/hvm/event.h
            index 11eb1fe..7c2252b 100644
            --- a/xen/include/asm-x86/hvm/event.h
            +++ b/xen/include/asm-x86/hvm/event.h
            @@ -27,9 +27,8 @@ bool_t hvm_event_cr(unsigned int index,
            unsigned long value,
              #define hvm_event_crX(what, new, old) \
                  hvm_event_cr(VM_EVENT_X86_##what, new, old)
              void hvm_event_msr(unsigned int msr, uint64_t value);
            -/* Called for current VCPU: returns -1 if no listener */
            -int hvm_event_int3(unsigned long rip);
            -int hvm_event_single_step(unsigned long rip);
            +int hvm_event_software_breakpoint(unsigned long rip,
            +                                  bool_t single_step);

        Are we liable to ever gain any other type of software
        breakpoint?  Might
        it be more sense to pass an enum rather than a bool here?

        Otherwise, the changes look sensible.

        ~Andrew


    Well, IMHO, no. Besides breakpointing/trapping after each
    instruction (i.e. VM_EVENT_REASON_SINGLESTEP)
    and on arbitrary instructions
    (VM_EVENT_REASON_SOFTWARE_BREAKPOINT) I don't see what other
    types of breakpointing one can define (at least from the
    hypervisor's point of view), and if in the future
    there will be other types, that could also be changed into an enum
    then.
    But making that param an enum now would also be fine by me.
    Since I noticed Tamas also prefers this option, I will make that
    change.


It's just about code readability. Functionally it would be the same as it is right now, two options in the enum will likely be represented by 0/1. But when you read the code you don't have to keep that boolean state in mind, you explicitly have the path spelled out.

Tamas


That makes sense, and probably that enum value will be handled in a switch.
Will do.

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

Reply via email to