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: Sun, 28 Nov 2010 11:31:09 +0200 Message-ID: <4CF2215D.9060201@redhat.com> References: <1290595933-13122-1-git-send-email-avi@redhat.com> <1290595933-13122-3-git-send-email-avi@redhat.com> <20101126101625.GA3657@redhat.com> <4CF0CB9A.5060403@redhat.com> <20101128085833.GA3330@redhat.com> 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: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:65434 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751927Ab0K1JbN (ORCPT ); Sun, 28 Nov 2010 04:31:13 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oAS9VCjk025472 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 28 Nov 2010 04:31:13 -0500 In-Reply-To: <20101128085833.GA3330@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 11/28/2010 10:58 AM, Michael S. Tsirkin wrote: > On Sat, Nov 27, 2010 at 11:12:58AM +0200, Avi Kivity wrote: > > On 11/26/2010 12:16 PM, Michael S. Tsirkin wrote: > > >On Wed, Nov 24, 2010 at 12:52:11PM +0200, Avi Kivity wrote: > > >> Introduce exception-safe objects for calling system, vm, and vcpu ioctls. > > >> > > >> Signed-off-by: Avi Kivity > > > > > >ioctlp calls below ignore possible errors. > > >Somre more comments below. > > > > > > > Can you elaborate? The simply propagate the exception. > > I was confused by this: > > + long ioctlp(unsigned nr, void *arg) { > + return ioctl(nr, reinterpret_cast(arg)); > + } > > > ioctl here is not the C ioctl function. It's the local method that > throws exceptions on errors. This will likely confuse others > as well. Could do this->ioctl(), though I don't much like it. > > >> + > > >> +std::vector vcpu::msrs(std::vector indices) > > >> +{ > > >> + std::auto_ptr msrs(alloc_msr_list(indices.size())); > > > > > >This looks wrong. auto_ptr frees memory with delete, > > >alloc_msr_list allocates it with malloc. > > > > Anthony pointed this out as well. > > Another problem is that there seem to be two memory allocations and a > copy here, apparently just to simplify error handling. It might be fine > for this test but won't scale for when performance matters. When it matters, we can fix it. I don't see msr read/write becoming a hot path. > > Fixed by replacing > > alloc_msr_list() by an object. > > It seems that any action which has side effects which needs to be > undone on error, we will have to have a new class with constructor doing > the work. This will likely create much more lines of code > than a simple goto end strategy. It creates correctness. The equivalent in qemu is to create a constant size array on the stack, because people can't be bothered with error checking. The lines of code pay back as soon as there is some reuse (2x in this case). > One also wonders how well will the compiler be able to optimize > such complex code, and by how much will compile times go up. > > Any input on that? The compiler should optimize it away completely. There's been a lot of work in gcc on that. About compile times, I don't care much. -- error compiling committee.c: too many arguments to function