From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm
 APIs
Date: Wed, 24 Nov 2010 08:45:08 -0600
Message-ID: <4CED24F4.6060300@codemonkey.ws>
References: <1290595933-13122-1-git-send-email-avi@redhat.com> <1290595933-13122-3-git-send-email-avi@redhat.com> <4CED1CC6.20603@codemonkey.ws> <4CED2148.60005@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Marcelo Tosatti <mtosatti@redhat.com>, kvm@vger.kernel.org
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-qw0-f46.google.com ([209.85.216.46]:55950 "EHLO
	mail-qw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750974Ab0KXOpp (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 24 Nov 2010 09:45:45 -0500
Received: by qwb7 with SMTP id 7so930568qwb.19
        for <kvm@vger.kernel.org>; Wed, 24 Nov 2010 06:45:44 -0800 (PST)
In-Reply-To: <4CED2148.60005@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 11/24/2010 08:29 AM, Avi Kivity wrote:
> On 11/24/2010 04:10 PM, Anthony Liguori wrote:
>> On 11/24/2010 04:52 AM, Avi Kivity wrote:
>>> Introduce exception-safe objects for calling system, vm, and vcpu 
>>> ioctls.
>>>
>>> +
>>> +namespace kvm {
>>> +
>>> +static long check_error(long r)
>>> +{
>>> +    if (r == -1) {
>>> +    throw errno;
>>> +    }
>>> +    return r;
>>> +}
>>
>> It's generally nicer for exceptions to be objects and to inherit from 
>> std::exception.  Otherwise, you can run into nasty issues when 
>> catching the wrong type.
>
>
> Yeah, I'm lazy.
>
>>
>>> +fd::fd(int fd)
>>> +    : _fd(fd)
>>> +{
>>> +}
>>
>> There's no need to prefix with '_'.  Every compiler has been able to 
>> do the right thing with : fd(fd) for a long time.
>
> Ok.
>
>>
>>> +
>>> +kvm_sregs vcpu::sregs()
>>> +{
>>> +    kvm_sregs sregs;
>>> +    _fd.ioctlp(KVM_GET_SREGS,&sregs);
>>> +    return sregs;
>>> +}
>>
>> I think you're missing an opportunity by just returning the 
>> structures in their raw form as opposed to wrapping them in an object.
>
> What would the object do besides adding tons of accessors?

I would think that you'd have a single object that represented the full 
CPU state and then you'd have methods that allowed individual groups to 
be refreshed.

Something like:

struct x86_vcpu : public vcpu
{
      uint64_t eax;
      uint64_t ebx;
      uint64_t ecx;
      //...

     void get_gps(void);
     void put_gps(void);
     void get_sregs(void);
     void put_sregs(void);

     std::string repr(void);
};

I'm not of the opinion that all members need getters and setters.  I 
think it's perfectly fine to have public variables if the reads and 
writes don't have side effects.

Regards,

Anthony Liguori

>
>>
>>> +
>>> +void vcpu::set_sregs(const kvm_sregs&  sregs)
>>> +{
>>> +    _fd.ioctlp(KVM_SET_SREGS, const_cast<kvm_sregs*>(&sregs));
>>> +}
>>> +
>>> +kvm_msrs* vcpu::alloc_msr_list(size_t nmsrs)
>>> +{
>>> +    size_t size = sizeof(kvm_msrs) + sizeof(kvm_msr_entry) * nmsrs;
>>> +    kvm_msrs* ret = static_cast<kvm_msrs*>(malloc(size));
>>> +    if (!ret) {
>>> +    throw ENOMEM;
>>> +    }
>>> +    return ret;
>>> +}
>>
>> malloc?
>>
>> Mixing C and C++ allocations is nasty stuff.  Would be nicer to new 
>> an object and return it such that delete can be used consistently.
>
> 5 years of C.
>
>>
>>> +
>>> +std::vector<kvm_msr_entry>  vcpu::msrs(std::vector<uint32_t>  indices)
>>> +{
>>> +    std::auto_ptr<kvm_msrs>  msrs(alloc_msr_list(indices.size()));
>>> +    msrs->nmsrs = indices.size();
>>> +    for (unsigned i = 0; i<  msrs->nmsrs; ++i) {
>>> +    msrs->entries[i].index = indices[i];
>>> +    }
>>> +    _fd.ioctlp(KVM_GET_MSRS, msrs.get());
>>> +    return std::vector<kvm_msr_entry>(msrs->entries,
>>> +                      msrs->entries + msrs->nmsrs);
>>> +}
>>
>> auto_ptr has pretty awful semantics.  tr1::shared_ptr is now available.
>
> For this it's perfectly fine.
>
>>> +
>>> +class fd {
>>> +public:
>>> +    explicit fd(int n);
>>> +    explicit fd(std::string path, int flags);
>>> +    fd(const fd&  other);
>>> +    ~fd() { ::close(_fd); }
>>> +    int get() { return _fd; }
>>> +    long ioctl(unsigned nr, long arg);
>>> +    long ioctlp(unsigned nr, void *arg) {
>>> +    return ioctl(nr, reinterpret_cast<long>(arg));
>>> +    }
>>
>> I think mixing inline definitions with declarations is bad form.
>
> It is, but on the other hand I hate the verbosity of all those little 
> accessors.
>