From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs Date: Wed, 24 Nov 2010 16:29:28 +0200 Message-ID: <4CED2148.60005@redhat.com> References: <1290595933-13122-1-git-send-email-avi@redhat.com> <1290595933-13122-3-git-send-email-avi@redhat.com> <4CED1CC6.20603@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Anthony Liguori Return-path: Received: from mx1.redhat.com ([209.132.183.28]:3545 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751708Ab0KXO3d (ORCPT ); Wed, 24 Nov 2010 09:29:33 -0500 In-Reply-To: <4CED1CC6.20603@codemonkey.ws> Sender: kvm-owner@vger.kernel.org List-ID: 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(&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(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 vcpu::msrs(std::vector indices) >> +{ >> + std::auto_ptr 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(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(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