Re: [Qemu-devel] [PATCH v5 03/10] qemu-iotests: automatically clean up bash protocol servers

[Paolo Bonzini]

On 18/10/2017 17:03, Jeff Cody wrote:
> On Wed, Oct 18, 2017 at 04:46:20PM +0200, Paolo Bonzini wrote:
>> On 17/10/2017 18:31, Jeff Cody wrote:
>>>              )
>>>          else
>>> +            # Do this in a sub-shell, so we are operating on the right
>>> +            # TEST_DIR / QEMU_TEST_DIR
>>>              (
>>>              export TEST_DIR=$TEST_DIR_SEQ
>>>              cd "$source_iotests";
>> Where is the missing ")"?
> It's part of the diff context, not a change itself... it's still there, just
> not shown in the patch.
> 
>>> @@ -837,6 +841,15 @@ do
>>>              fi
>>>          fi
>>>  
>>> +        # Do this in a sub-shell, so we are operating on the right
>>> +        # TEST_DIR / QEMU_TEST_DIR
>>> +        (
>>> +        export TEST_DIR=$TEST_DIR_SEQ
>>> +        . "$source_iotests/common.config"
>>> +        . "$source_iotests/common.rc"
>>> +
>>> +        _cleanup_protocols
>> "check" wasn't including common.rc before this patch, and most of the
>> content of that file doesn't apply to "check".  So if we want to move
>> cleanup code to "check" we should remove it from common.rc too.
>>
>> In general, I think we should strive to have a better separation between
>> tests and harness.  This for example could let us write a different
>> harness for the same tests and integrate the tests better into Avocado.
>> Hence I see one advantage and one disadvantage in your series:
>>
>> * by adding more functionality to "check", it shows that the current
>> separation may fail with a more sophisticated harness (such as the one
>> you are creating here)
>>
>> * it adds a lot more knowledge of QEMU (especially protocols) in
>> "check", but there is still some unbalance: tests create the images and
>> the protocol servers, but the harness cleans it up.  The visible result
>> of this unbalance is for example how multi-process protocol tests can
>> fail when multiple tests try to bind the same address.
>>
>> I think it's the right direction, but it feels like it's not there
>> yet...  Sorry---I know this is not very constructive, but I hope it
>> helps anyway.
>>
>> Maybe we should actually rewrite "check" in Python.  That would force us
>> to think more about the design.
>
> I think writing a more sophisticated harness in another language, such as
> Python, has merit.  But to clarify, do you mean that as a 'nack' to this
> series, or as something to be done later, after this series?  If this series
> improves the existing harness, I don't see why to exclude it because a new

Well, I have no idea (hence the "not very constructive" part).  I'm only
"nacking" the sourcing of common.rc in the check script.

The series improves the harness, but it also sets a very different
separation between the tests and the harness (especially WRT the tests
cleaning up after themselves).  The level of separation would at least
be clearer if check didn't include common.rc.

Paolo