Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs

[Avi Kivity <avi@redhat.com>]

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?

>
>> +
>> +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.

-- 
error compiling committee.c: too many arguments to function