Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices

[Thomas Huth]

On 07/12/2020 17.30, Cornelia Huck wrote:
> On Mon, 7 Dec 2020 15:28:47 +0100
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 04/12/2020 13.14, Cornelia Huck wrote:
>>> Hotplug a virtio-net-ccw device, and then hotunplug it again.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>>
>>> v1->v2:
>>> - switch device id
>>> - clear out dmesg before looking for CRW messages
>>>
>>> ---
>>>  tests/acceptance/machine_s390_ccw_virtio.py | 16 ++++++++++++++++
>>>  1 file changed, 16 insertions(+)
>>>
>>> diff --git a/tests/acceptance/machine_s390_ccw_virtio.py 
>>> b/tests/acceptance/machine_s390_ccw_virtio.py
>>> index 53b8484f8f9c..83c00190621b 100644
>>> --- a/tests/acceptance/machine_s390_ccw_virtio.py
>>> +++ b/tests/acceptance/machine_s390_ccw_virtio.py
>>> @@ -97,3 +97,19 @@ class S390CCWVirtioMachine(Test):
>>>          exec_command_and_wait_for_pattern(self,
>>>                                            'cat 
>>> /sys/bus/pci/devices/000a\:00\:00.0/function_id',
>>>                                            '0x0000000c')
>>> +        # add another device
>>> +        exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ')
>>> +        self.vm.command('device_add', driver='virtio-net-ccw',
>>> +                        devno='fe.0.4711', id='net_4711')
>>> +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')  
>>
>> Looking at this twice, I'm a little bit afraid that this could be racy -
>> what if the kernel decides to emit the line with the "CRW" just after we
>> executed the dmesg command? I'd maybe use something like this instead:
>>
>> exec_command_and_wait_for_pattern(self,
>>  'while ! dmesg -c | grep CRW ; do sleep 1 ; done', '~ #')
> 
> Yes, you're right. Unless anyone can think of a better incantation?

You could maybe drop the sleep 1 to avoid delays... but that might burn more
CPU cycles, so not sure what's better here?

(Unfortunately, that "sleep" in the initrd does not support fractions,
otherwise I'd suggest "sleep 0.1" instead)

>>
>>> +        exec_command_and_wait_for_pattern(self, 'ls /sys/bus/ccw/devices/',
>>> +                                          '0.0.4711')
>>> +        # and detach it again
>>> +        exec_command_and_wait_for_pattern(self, 'dmesg -c', ' ')  
>>
>> If adapt my above change, you could also get rid of this dmesg -c here
>> (since it's done in the while loop already)
> 
> I don't think so (there are two CRWs posted, and the loop might have
> caught the first one only.)

Oh, you're right. So let's better be safe than sorry and keep this dmesg -c.

 Thomas