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

[Anthony Liguori <anthony@codemonkey.ws>]

On 11/28/2010 06:27 AM, Michael S. Tsirkin wrote:
> On Wed, Nov 24, 2010 at 08:41:26AM -0600, Anthony Liguori wrote:
>    
>> On 11/24/2010 06:59 AM, Alexander Graf wrote:
>>      
>>> On 24.11.2010, at 11:52, Avi Kivity wrote:
>>>
>>>        
>>>> Introduce exception-safe objects for calling system, vm, and vcpu ioctls.
>>>>
>>>> Signed-off-by: Avi Kivity<avi@redhat.com>
>>>>          
>>> FWIW, I still disagree with C++ and believe this code to be hardly readable.
>>>        
>> There's a general prettiness that well written C++ code will have
>> over C when there's heavy object modelling.  This can be subjective
>> but for me, it's fairly significant.
>>      
> My guess is this comes from the fact that you are rewriting large pieces
> of code from scratch so it suits your personal style perfectly :)
>    

That's probably a good observations.

> If history teaches us anything, as with most projects in qemu, what we will
> end up with is a half done conversion of maybe 30% of the codebase.
> The result might be anything: safer, more correct - but it won't be prettier.
>    

This is where things need to be different.  I'm not at all interested in 
"introducing" C++ to QEMU because of exactly what you describe above.

I think the only viable approach is one where we have a segregated code 
base that is correct with an incremental movement of code from the "old" 
code base to the new way of doing things.

I've always thought that the device model should be a library and I 
think that's the way to structure it.  Have a libqemuhw and only move 
devices into it as they are converted properly.

>> The fact that objects are easily created on the stack and on the
>> heap is also pretty significant.
>>      
> Significant how?
>
> To create an object on the stack, you must have the class definition in
> a public header and a public constructor/destructor.
> This is exactly the same in C.
>    

It's really more of a design statement than a statement about C++ vs. C.

In qdev today, we mix object initialization with a user-exposed 
factory.  This means that you cannot do something simple like:

struct i440fx {
    struct piix3 piix;
};

void i440fx_init(struct i440fx_init *obj) {
     piix3_init(&obj->piix);
}

But rather need to use ugly factory functions with all sorts of 
DO_UPCAST.  This is really unfriendly especially for writing test cases.

But this isn't C vs. C++, this is just about device model design.  I 
think C++ makes it quite a bit more obvious though how to design 
correctly though.

>> In real hardware, the i8042 (keyboard controller) is actually
>> implemented in the PIIX3 which is a chip that is part of the i440fx.
>> The i440fx acts as both the memory controller and as the PCI Host
>> controller.  So you get something that looks like:
>>
>> class PIIX3 : public PCIDevice
>> {
>> private:
>>      I8042 i8042;
>>      RTC rtc;
>>      // ...
>> };
>>
>> class I440FX : public PCIHostController
>> {
>>     I440FX(void) {
>>          this->slots[1].plug(&this->piix3); // piix3 is always in slot 1
>>     }
>>
>> private:
>>     Plug<PCIDevice *>  slots[32]; // slot 0 is the PMC
>>     PIIX3 piix3;
>> };
>>      
>
> We can have the same thing today.  In fact, getting rid of the UP_CAST
> and opaque pointers should be a priority.
>    

I find that you end up writing a lot more boiler plate code trying to do 
this properly in C.   I think gobject is probably the best example of 
this.  If you've ever written a GTK widget from scratch in C and then 
written one in gtkmm, the difference is night and day.

>> So whereas we have this very complicate machine create function that
>> attempts to create and composite all of these devices after the
>> fact, when written in C++, partially due to good design, but
>> partially due to the fact that the languages forces you to think a
>> certain way, you get a tremendous simplification.
>>
>> A proper C++ device model turns a vast majority of our device
>> creation complexity into a single new I440FX.  Then it's just a
>> matter of instantiating and plugging the appropriate set of PCI
>> devices.
>>
>> Of course, this can be wrapped in a factory to make it drivable via
>> an API or config file.
>>
>> Another area that C++ shines is safety.  C++ enables you to inject
>> safe versions of things that you really can't do in C.  For
>> instance, the PIT has three channels but the mask to select a
>> channel is two bits.  There was a kernel exploit that found a way to
>> trick selection of a forth channel because of a missing check.
>>
>> In C++, you can convert:
>>
>> PITChannel channnels[3];
>>
>> Into:
>>
>> Array<PITChannel, 3>  channels;
>>
>> It behaves in every other way just like a normal array.  The memory
>> is stack allocated, the type has a fixed size.   The only difference
>> is that you can overload the [] operators and implement bounds
>> checking for array accesses.  This means that as long as you use
>> Array<>, array overflows disappear from the code base.  That's a big
>> deal.
>>      
> Except that you get used to the fact that [] is safe,
> and then forget to check the value in a dynamically
> sized array access. Boom.
>    

I don't think the fact that you get deterministic vs. non-deterministic 
behavior changes the approach people take to using arrays.  If it simply 
threw away invalid accesses, I might buy your argument.  But anyone that 
catches an out of bound exception as a normal error handling mechanism 
deserves to get beaten with a three day old trout.

>> Another area C++ shines is generating metacode.  Consider the
>> ugliness around VMState.  The crux of the problem is that it's not
>> possible to write type-neutral code in C.  This all gets simplified
>> with C++.  Instead of having a bunch of macros like:
>>
>> VMSTATE_INT8(val0, ...)
>> VMSTATE_INT16(val1, ...)
>>
>> You can just have:
>>
>> vmstate(val0)
>> vmstate(val1)
>>
>> And use type overloading to implement different behaviors.  Combined
>> with template specialization and an Array wrapper, the same thing
>> works for arrays too.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>> Regards,
>>
>> Anthony Liguori
>>      
> At least with VMSTATE_INT16 I can grep and find the definition.
>    

grep 'vmstate(.*, int16_t' *.hpp

Works perfectly fine.

Regards,

Anthony Liguori

>    
>>> Alex
>>>
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>        
>> --
>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>