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

Anthony Liguori
2010-11-24T14:45:45Z

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