[Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Paul Brook]

> From: Anthony Liguori <aliguori@us.ibm.com>
>
> This cleans up quite a lot of #ifdefs, extern variables, and other
> ugliness.

This changes the default for at least the ARM target, which I consider to be a 
bug. Worse than that, the default is now arbitrary and depends on unspecified 
toolchain implementation details.

Paul

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Glauber Costa]

On Thu, May 21, 2009 at 11:36:01PM +0100, Paul Brook wrote:
> > From: Anthony Liguori <aliguori@us.ibm.com>
> >
> > This cleans up quite a lot of #ifdefs, extern variables, and other
> > ugliness.
> 
> This changes the default for at least the ARM target, which I consider to be a 
> bug. Worse than that, the default is now arbitrary and depends on unspecified 
> toolchain implementation details.
How about we start sending patches to the list? Then this kind of thing can be avoided.

Note that although at first there is nothing wrong with just messing around with
the devel repository, this kind of thing breaks bisectability of the tree, which is kind 
of a pain.

c'mon guys, patches to the list.
Let's start doing it before IBM put a patent on it.

[Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Anthony Liguori]

Paul Brook wrote:
>> From: Anthony Liguori <aliguori@us.ibm.com>
>>
>> This cleans up quite a lot of #ifdefs, extern variables, and other
>> ugliness.
>>     
>
> This changes the default for at least the ARM target, which I consider to be a 
> bug. Worse than that, the default is now arbitrary and depends on unspecified 
> toolchain implementation details.
>   

The default depends on order?  That's unfortunate.  I'll fix it.

> Paul
>   


-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Anthony Liguori]

Glauber Costa wrote:
> On Thu, May 21, 2009 at 11:36:01PM +0100, Paul Brook wrote:
>   
>>> From: Anthony Liguori <aliguori@us.ibm.com>
>>>
>>> This cleans up quite a lot of #ifdefs, extern variables, and other
>>> ugliness.
>>>       
>> This changes the default for at least the ARM target, which I consider to be a 
>> bug. Worse than that, the default is now arbitrary and depends on unspecified 
>> toolchain implementation details.
>>     
> How about we start sending patches to the list? Then this kind of thing can be avoided.
>   

It doesn't magically solve the problem.  I post most patches and still 
regressions slip in.  I review every patch I commit and still 
regressions slip in.  People are imperfect.

The best way to prevent regressions is to have an automated test suite 
that everyone can use to validate that a series of patches doesn't break 
things.

> Note that although at first there is nothing wrong with just messing around with
> the devel repository, this kind of thing breaks bisectability of the tree, which is kind 
> of a pain.
>   

You can always --skip.  I understand your point.  In this case, the 
patch was very large and mostly mechanical.  There was a design flaw but 
I didn't expect to get much useful feedback because of the shear amount 
of things it touched.

Regression tests are the only way we're going to really solve this problem.

-- 
Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Glauber Costa]

On Thu, May 21, 2009 at 08:34:48PM -0500, Anthony Liguori wrote:
> Glauber Costa wrote:
>> On Thu, May 21, 2009 at 11:36:01PM +0100, Paul Brook wrote:
>>   
>>>> From: Anthony Liguori <aliguori@us.ibm.com>
>>>>
>>>> This cleans up quite a lot of #ifdefs, extern variables, and other
>>>> ugliness.
>>>>       
>>> This changes the default for at least the ARM target, which I 
>>> consider to be a bug. Worse than that, the default is now arbitrary 
>>> and depends on unspecified toolchain implementation details.
>>>     
>> How about we start sending patches to the list? Then this kind of thing can be avoided.
>>   
>
> It doesn't magically solve the problem.  I post most patches and still  
> regressions slip in.  I review every patch I commit and still  
> regressions slip in.  People are imperfect.
true.

>
> The best way to prevent regressions is to have an automated test suite  
> that everyone can use to validate that a series of patches doesn't break  
> things.
true...

>
>> Note that although at first there is nothing wrong with just messing around with
>> the devel repository, this kind of thing breaks bisectability of the 
>> tree, which is kind of a pain.
>>   
>
> You can always --skip.  I understand your point.  In this case, the  
> patch was very large and mostly mechanical.  There was a design flaw but  
> I didn't expect to get much useful feedback because of the shear amount  
> of things it touched.

however, for a lot of patches that recently went in, there were discussions
_after_ the patch made its way to the repository. The discussions help, everybody
does that. Giving people a chance to stand up and raise valid points before
a change is made to the repository is at the very least, a polite attitude to be
taken. And although I agree with you that it does not solve all problems, it
really does help improving the situation by a huge leap.

Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions

[Anthony Liguori]

Glauber Costa wrote:
>>> Note that although at first there is nothing wrong with just messing around with
>>> the devel repository, this kind of thing breaks bisectability of the 
>>> tree, which is kind of a pain.
>>>   
>>>       
>> You can always --skip.  I understand your point.  In this case, the  
>> patch was very large and mostly mechanical.  There was a design flaw but  
>> I didn't expect to get much useful feedback because of the shear amount  
>> of things it touched.
>>     
>
> however, for a lot of patches that recently went in, there were discussions
> _after_ the patch made its way to the repository. The discussions help, everybody
> does that. Giving people a chance to stand up and raise valid points before
> a change is made to the repository is at the very least, a polite attitude to be
> taken. And although I agree with you that it does not solve all problems, it
> really does help improving the situation by a huge leap.
>   

I appreciate where you're coming from.  I've made the same argument in 
the context of other projects.

Practically speaking, very few projects have every single patch go to 
the mailing list first.  Almost always, quick fixes or trivial things 
are committed directly by the maintainers.  I think the real balancing 
act is determine what falls into the category of quick fixes/trivial 
changes and what ought to go to the list.

So please do provide feedback on particular commits that you think 
should have gone to the mailing list.  I don't think there will every be 
an every single change to the mailing list first policy, but it's 
certainly open to discussion about what changes should get review first.

-- 
Regards,

Anthony Liguori

autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Dor Laor]

Anthony Liguori wrote:
> Glauber Costa wrote:    
>> How about we start sending patches to the list? Then this kind of 
>> thing can be avoided.
>>   
>
> It doesn't magically solve the problem.  I post most patches and still 
> regressions slip in.  I review every patch I commit and still 
> regressions slip in.  People are imperfect.
>
> The best way to prevent regressions is to have an automated test suite 
> that everyone can use to validate that a series of patches doesn't 
> break things.
We have such a frame work, it's the kvm autotest project - 
http://www.linux-kvm.org/page/KVM-Autotest.
It is used currently by Avi for his tree + to test more internal trees. 
It recently catched a regression with -net usage.
Don't let its name fool you, it should be able to test bare qemu code 
too. Also we are finally on track for
merging the changes upstream towards autotest project.

So ideally, what we need for qemu project is:
1. Make kvm-autotest work with qemu only
    - It's mainly about changing timeouts and better, move to kickstart 
files
2. Have qemu community use it and contribute to it.
3. Maintainers should ...
    - Well, I'll let you maintainers complete that line :)

#1 will surely happen soon. I hope #2, #3 will join right after, making 
qemu head/stable a better place.
Regards,
Dor

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Anthony Liguori]

Dor Laor wrote:
> Anthony Liguori wrote:
>> Glauber Costa wrote:   
>>> How about we start sending patches to the list? Then this kind of 
>>> thing can be avoided.
>>>   
>>
>> It doesn't magically solve the problem.  I post most patches and 
>> still regressions slip in.  I review every patch I commit and still 
>> regressions slip in.  People are imperfect.
>>
>> The best way to prevent regressions is to have an automated test 
>> suite that everyone can use to validate that a series of patches 
>> doesn't break things.
> We have such a frame work, it's the kvm autotest project - 
> http://www.linux-kvm.org/page/KVM-Autotest.

Autotest doesn't currently test the regressions that I want to test.  
But yeah, I agree with the general point that whatever we use as a 
regression suite ought to be drivable by autotest.

Regards,

Anthony Liguori

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Avi Kivity]

Anthony Liguori wrote:
> Autotest doesn't currently test the regressions that I want to test.  

Which regressions do you want to test?

One way to increase coverage is to require each patch (fix or feature) 
to come with a test.  On the other hand, it might stop contributions 
instead of increasing test coverage.

I've used this method for the x86 emulator in kvm, with some success.

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

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Anthony Liguori]

Avi Kivity wrote:
> Anthony Liguori wrote:
>> Autotest doesn't currently test the regressions that I want to test.  
>
> Which regressions do you want to test?

For instance, we often have networking regressions.  In particular, with 
the refactoring going on in slirp and tun/tap, I'd really like to have 
an automated way to test slirp and tap with TCP transmit/receive, and 
slirp redir.  I have something locally to do this that I intend on 
posting in the next few days.

>
> One way to increase coverage is to require each patch (fix or feature) 
> to come with a test.  On the other hand, it might stop contributions 
> instead of increasing test coverage.
>
> I've used this method for the x86 emulator in kvm, with some success.

I wouldn't require kvm-autotest, but I'd like to have an interest set of 
unit tests that are then usable within kvm-autotest.  Once we have that, 
asking politely for tests to be written I think is quite reasonable.

Regards,

Anthony Liguori

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Avi Kivity]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> Autotest doesn't currently test the regressions that I want to test.  
>>
>> Which regressions do you want to test?
>
> For instance, we often have networking regressions.  In particular, 
> with the refactoring going on in slirp and tun/tap, I'd really like to 
> have an automated way to test slirp and tap with TCP transmit/receive, 
> and slirp redir.  

kvm-autotest does test slirp (and redirection).  It doesn't test tap 
AFAIK, but could easily be extended to do so.

-- 
error compiling committee.c: too many arguments to function

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Dor Laor]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Anthony Liguori wrote:
>>> Autotest doesn't currently test the regressions that I want to test.  
>>
>> Which regressions do you want to test?
>
> For instance, we often have networking regressions.  In particular, 
> with the refactoring going on in slirp and tun/tap, I'd really like to 
> have an automated way to test slirp and tap with TCP transmit/receive, 
> and slirp redir.  I have something locally to do this that I intend on 
> posting in the next few days.
>
Current kvm autotest do test slirp. The step-files for guest 
installation open ssh/telnet access in order
to let the host reach them.
We trapped exactly this regression using autotest.
>>
>> One way to increase coverage is to require each patch (fix or 
>> feature) to come with a test.  On the other hand, it might stop 
>> contributions instead of increasing test coverage.
>>
>> I've used this method for the x86 emulator in kvm, with some success.
>
> I wouldn't require kvm-autotest, but I'd like to have an interest set 
> of unit tests that are then usable within kvm-autotest.  Once we have 
> that, asking politely for tests to be written I think is quite 
> reasonable.
Some features are easily tested using a large framework, for example 
migration, slirp, time drift, etc.
In addition kvm's tests unit tests, qemu-io and similar should be 
written too in order to test various cases that
are only reasonable to be found using low level operations.
No doubt all of the unit tests should be executed from autotest.
>
> Regards,
>
> Anthony Liguori
>

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Anthony Liguori]

Dor Laor wrote:
> Anthony Liguori wrote:
>> Avi Kivity wrote:
>>> Anthony Liguori wrote:
>>>> Autotest doesn't currently test the regressions that I want to test.  
>>>
>>> Which regressions do you want to test?
>>
>> For instance, we often have networking regressions.  In particular, 
>> with the refactoring going on in slirp and tun/tap, I'd really like 
>> to have an automated way to test slirp and tap with TCP 
>> transmit/receive, and slirp redir.  I have something locally to do 
>> this that I intend on posting in the next few days.
>>
> Current kvm autotest do test slirp. The step-files for guest 
> installation open ssh/telnet access in order
> to let the host reach them.
> We trapped exactly this regression using autotest.

What I want is a single test that completes quickly.  This means a 
preconfigured guest that I can run a quick test case against.

Installation tests a lot of things.  I'm looking for more targeted tests.

>> I wouldn't require kvm-autotest, but I'd like to have an interest set 
>> of unit tests that are then usable within kvm-autotest.  Once we have 
>> that, asking politely for tests to be written I think is quite 
>> reasonable.
> Some features are easily tested using a large framework, for example 
> migration, slirp, time drift, etc.
> In addition kvm's tests unit tests, qemu-io and similar should be 
> written too in order to test various cases that
> are only reasonable to be found using low level operations.
> No doubt all of the unit tests should be executed from autotest.

Yup, I agree 100%.

Regards,

Anthony Liguori

Re: autotest (was Re: [Qemu-devel] Re: [Qemu-commits] [COMMIT f80f9ec] Convert machine registration to use module initfunctions)

[Dor Laor]

Anthony Liguori wrote:
> Dor Laor wrote:
>> Anthony Liguori wrote:
>>> Avi Kivity wrote:
>>>> Anthony Liguori wrote:
>>>>> Autotest doesn't currently test the regressions that I want to test.  
>>>>
>>>> Which regressions do you want to test?
>>>
>>> For instance, we often have networking regressions.  In particular, 
>>> with the refactoring going on in slirp and tun/tap, I'd really like 
>>> to have an automated way to test slirp and tap with TCP 
>>> transmit/receive, and slirp redir.  I have something locally to do 
>>> this that I intend on posting in the next few days.
>>>
>> Current kvm autotest do test slirp. The step-files for guest 
>> installation open ssh/telnet access in order
>> to let the host reach them.
>> We trapped exactly this regression using autotest.
>
> What I want is a single test that completes quickly.  This means a 
> preconfigured guest that I can run a quick test case against.
>
> Installation tests a lot of things.  I'm looking for more targeted tests.
No doubt unit tests and some tools run manually are the best.
Nevertheless, the projects needs fully automated test framework that 
test it all.
Even today you do not have to go through the guest install each time.
Guest images are not erased after the install and autotest itself spawn 
additional
tests over this image. Even a VM can be kept alive at the end of a test, 
waiting for
additional tests to be executed over it.

In the future, I envision that you can go to auto-serv web interface, 
manually pick
a host and a test case and run it. The test will have built-in 
dependencies, and
if the VM is installed and alive it would just run the test as quick as 
possible.
Otherwise it will automatically do the dependency chain (or prompt for it).
>
>>> I wouldn't require kvm-autotest, but I'd like to have an interest 
>>> set of unit tests that are then usable within kvm-autotest.  Once we 
>>> have that, asking politely for tests to be written I think is quite 
>>> reasonable.
>> Some features are easily tested using a large framework, for example 
>> migration, slirp, time drift, etc.
>> In addition kvm's tests unit tests, qemu-io and similar should be 
>> written too in order to test various cases that
>> are only reasonable to be found using low level operations.
>> No doubt all of the unit tests should be executed from autotest.
>
> Yup, I agree 100%.
>
> Regards,
>
> Anthony Liguori