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

["Michael S. Tsirkin" <mst@redhat.com>]

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<avi@redhat.com>
> >>  >
> >>  >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<long>(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<kvm_msr_entry>   vcpu::msrs(std::vector<uint32_t>   indices)
> >>  >>   +{
> >>  >>   +    std::auto_ptr<kvm_msrs>   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