From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH kvm-unit-tests 2/4] Introduce a C++ wrapper for the kvm APIs Date: Sun, 28 Nov 2010 11:50:56 +0200 Message-ID: <20101128095056.GA4499@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> <4CF2215D.9060201@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , kvm@vger.kernel.org To: Avi Kivity Return-path: Received: from mx1.redhat.com ([209.132.183.28]:53330 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750779Ab0K1JvP (ORCPT ); Sun, 28 Nov 2010 04:51:15 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id oAS9pFqK028390 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sun, 28 Nov 2010 04:51:15 -0500 Content-Disposition: inline In-Reply-To: <4CF2215D.9060201@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sun, Nov 28, 2010 at 11:31:09AM +0200, Avi Kivity wrote: > 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. It will be very painful to fix it. > >> 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. Should as opposed to does. Want me to try a simple test? > There's been a lot > of work in gcc on that. > > About compile times, I don't care much. I do. You will too when we have codebase that can be built as fast as we commit things, so buildbot breaks. This is common in C++ based projects. > -- > error compiling committee.c: too many arguments to function