Re: [Qemu-devel] [PATCH] kvm: add set_one_reg/get_one_reg

[Paolo Bonzini]

Il 18/09/2013 10:08, Alexey Kardashevskiy ha scritto:
> On 09/18/2013 05:59 PM, Peter Maydell wrote:
>> On 18 September 2013 16:46, Alexey Kardashevskiy <address@hidden> wrote:
>>> On 09/18/2013 05:14 PM, Peter Maydell wrote:
>>>> On 18 September 2013 05:21, Alexey Kardashevskiy <address@hidden> wrote:
>>>>> This adds QEMU wrappers for KVM_SET_ONE_REG/KVM_GET_ONE_REG ioctls.
>>>>>
>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>>>> ---
>>>>>  include/sysemu/kvm.h |  4 ++++
>>>>>  kvm-all.c            | 31 +++++++++++++++++++++++++++++++
>>>>>  2 files changed, 35 insertions(+)
>>>>>
>>>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>>>> index c7bc07b..b2d61e9 100644
>>>>> --- a/include/sysemu/kvm.h
>>>>> +++ b/include/sysemu/kvm.h
>>>>> @@ -319,4 +319,8 @@ int kvm_irqchip_remove_irqfd_notifier(KVMState *s, 
>>>>> EventNotifier *n, int virq);
>>>>>  void kvm_pc_gsi_handler(void *opaque, int n, int level);
>>>>>  void kvm_pc_setup_irq_routing(bool pci_enabled);
>>>>>  void kvm_init_irq_routing(KVMState *s);
>>>>> +
>>>>> +int kvm_set_one_reg(CPUState *cs, uint64_t id, void *addr);
>>>>> +int kvm_get_one_reg(CPUState *cs, uint64_t id, void *addr);
>>>>
>>>> Doc comments, please.
>>>
>>> What comments? Do not the function names speak for themselves already?
>>
>> It's a new public function, it should have a comment in the standard
>> format saying what it does and what the input parameters and output are.
>> Just to pick some examples at random, the fact that the return value
>> is 0-on-success-or-negative-errno should be documented, and we should
>> refer the user to the ioctl docs for valid id values. It doesn't need to be
>> a long essay, but we should be documenting new functions as we add them.
> 
> 
> It would be awesome if you just gave me any really good example of what you
> expect from such a comment as kvm-all.c does not have any whatsoever. And
> pci.c does not. And exec.c does not. But I am sure there is some as
> it cannot possibly be me who starts making such comments in qemu. Thanks.

include/exec/memory.h, include/qom/object.h, include/block/blockjob.h,
include/qemu/main-loop.h, include/block/aio.h have many, thankfully.

Paolo