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

Avi Kivity
2010-11-24T14:29:33Z

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