Re: [Qemu-devel] [Qemu-ppc] [RFC PATCH] tests: Add a device_add/del HMP test

[Eduardo Habkost]

On Wed, Sep 13, 2017 at 07:45:11AM +0200, Thomas Huth wrote:
> On 12.09.2017 19:37, Eduardo Habkost wrote:
> > On Mon, Sep 11, 2017 at 08:13:21AM +0200, Thomas Huth wrote:
> >> On 09.09.2017 22:41, Eduardo Habkost wrote:
> >>> On Wed, Sep 06, 2017 at 08:59:32AM +0200, Markus Armbruster wrote:
> >>>> Thomas Huth <address@hidden> writes:
> >>>>
> >>>>> On 05.09.2017 18:48, Dr. David Alan Gilbert wrote:
> >>>>>> * Markus Armbruster (address@hidden) wrote:
> >>>>>>> Thomas Huth <address@hidden> writes:
> >>>>>>>
> >>>>>>>> People tend to forget to mark internal devices with "user_creatable 
> >>>>>>>> = false
> >>>>>>>> or hotpluggable = false, and these devices can crash QEMU if added 
> >>>>>>>> via the
> >>>>>>>> HMP monitor. So let's add a test to run through all devices and that 
> >>>>>>>> tries
> >>>>>>>> to add them blindly (without arguments) to see whether this could 
> >>>>>>>> crash the
> >>>>>>>> QEMU instance.
> >> [...]
> >>>>>>> * The device supports only cold plug with -device, not hot plug with
> >>>>>>>   device_add.
> >>>>>
> >>>>> We've got Eduardo's scripts/device-crash-test script for that already,
> >>>>> so no need to cover that here.
> >>>>
> >>>> Point taken.  So this test is really about hot plug / unplug.  Suggest
> >>>> to clarify the commit message: s/add them blindly/hotplug and unplug
> >>>> them blindly/.
> >>>
> >>> We could extend device-crash-test to test device_add too, as it
> >>> already has extra code to deal with known crashes and testing
> >>> multiple machine-types.  Also, any additional code we write to
> >>> ensure we add mandatory arguments or plug only to valid buses
> >>> would apply to both -device and device_add.  I also think Python
> >>> test code is easier to maintain and extend, but that's just my
> >>> personal preference.
> >>
> >> Adding device_add/del support to device-crash-test is certainly an
> >> option. The problem is that nobody runs it by default, so this won't
> >> help to avoid that new problems are being committed to the repository.
> >>
> >> I think we really should have a test for "make check", too. So would my
> >> test be acceptable if I'd rewrite it to use QMP instead (I don't think I
> >> could do the full list that Markus mentioned, but at least a basic test
> >> via QMP as a start)?
> > 
> > We can run device-crash-test on "make check", we just need to
> > choose what's the subset of tests we want to run (because testing
> > all machine+device+target combinations would take too long).
> 
> Maybe we should just run it one time for every machine - and try to add
> all available devices at once?

Yes, it makes sense.  I will keep that in mind when trying to
implement device_add support on device-crash-test (but if anybody
wants to volunteer to implement it, be my guest).

-- 
Eduardo