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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
