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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux