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

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

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.


> >>  +
> >>  +vcpu::~vcpu()
> >>  +{
> >>  +    unsigned mmap_size = _vm._system._fd.ioctl(KVM_GET_VCPU_MMAP_SIZE, 0);
> >
> >This might throw an exception on destructor path, if this happens
> >because an exception was thrown terminate is called.
> 
> Fixed.
> 
> >>  +
> >>  +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.

>  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.

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?

> 
> -- 
> 
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.