qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
@ 2020-12-04 12:14 Cornelia Huck
  2020-12-04 14:05 ` Wainer dos Santos Moschetta
  2020-12-07 14:28 ` Thomas Huth
  0 siblings, 2 replies; 10+ messages in thread
From: Cornelia Huck @ 2020-12-04 12:14 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Cornelia Huck, Wainer dos Santos Moschetta,
	Halil Pasic, Christian Borntraeger, qemu-s390x, Cleber Rosa,
	Philippe Mathieu-Daudé

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')
+        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', ' ')
+        self.vm.command('device_del', id='net_4711')
+        self.vm.event_wait(name='DEVICE_DELETED',
+                           match={'data': {'device': 'net_4711'}})
+        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')
+        exec_command_and_wait_for_pattern(self,
+                                          'ls /sys/bus/ccw/devices/0.0.4711',
+                                          'No such file or directory')
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-04 12:14 [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices Cornelia Huck
@ 2020-12-04 14:05 ` Wainer dos Santos Moschetta
  2020-12-04 14:08   ` Cornelia Huck
  2020-12-07 14:28 ` Thomas Huth
  1 sibling, 1 reply; 10+ messages in thread
From: Wainer dos Santos Moschetta @ 2020-12-04 14:05 UTC (permalink / raw)
  To: Cornelia Huck, qemu-devel
  Cc: Thomas Huth, Halil Pasic, Christian Borntraeger, qemu-s390x,
	Cleber Rosa, Philippe Mathieu-Daudé

Hi,

On 12/4/20 9:14 AM, 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', ' ')


The problem is that `dmesg -c` will fail if you run the test with 
unprivileged user.

- Wainer

> +        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')
> +        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', ' ')
> +        self.vm.command('device_del', id='net_4711')
> +        self.vm.event_wait(name='DEVICE_DELETED',
> +                           match={'data': {'device': 'net_4711'}})
> +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')
> +        exec_command_and_wait_for_pattern(self,
> +                                          'ls /sys/bus/ccw/devices/0.0.4711',
> +                                          'No such file or directory')



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-04 14:05 ` Wainer dos Santos Moschetta
@ 2020-12-04 14:08   ` Cornelia Huck
  2020-12-04 20:13     ` Wainer dos Santos Moschetta
  0 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2020-12-04 14:08 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: Thomas Huth, qemu-devel, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Cleber Rosa, Philippe Mathieu-Daudé

On Fri, 4 Dec 2020 11:05:34 -0300
Wainer dos Santos Moschetta <wainersm@redhat.com> wrote:

> Hi,
> 
> On 12/4/20 9:14 AM, 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', ' ')  
> 
> 
> The problem is that `dmesg -c` will fail if you run the test with 
> unprivileged user.

Hm, why should that make a difference for a guest command?

> 
> - Wainer
> 
> > +        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')
> > +        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', ' ')
> > +        self.vm.command('device_del', id='net_4711')
> > +        self.vm.event_wait(name='DEVICE_DELETED',
> > +                           match={'data': {'device': 'net_4711'}})
> > +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')
> > +        exec_command_and_wait_for_pattern(self,
> > +                                          'ls /sys/bus/ccw/devices/0.0.4711',
> > +                                          'No such file or directory')  



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-04 14:08   ` Cornelia Huck
@ 2020-12-04 20:13     ` Wainer dos Santos Moschetta
  2020-12-07 11:12       ` Cornelia Huck
  0 siblings, 1 reply; 10+ messages in thread
From: Wainer dos Santos Moschetta @ 2020-12-04 20:13 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: Thomas Huth, qemu-devel, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Cleber Rosa, Philippe Mathieu-Daudé


On 12/4/20 11:08 AM, Cornelia Huck wrote:
> On Fri, 4 Dec 2020 11:05:34 -0300
> Wainer dos Santos Moschetta <wainersm@redhat.com> wrote:
>
>> Hi,
>>
>> On 12/4/20 9:14 AM, 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', ' ')
>>
>> The problem is that `dmesg -c` will fail if you run the test with
>> unprivileged user.
> Hm, why should that make a difference for a guest command?


Never mind, my brain mix host and guest very often....

Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>


>
>> - Wainer
>>
>>> +        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')
>>> +        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', ' ')
>>> +        self.vm.command('device_del', id='net_4711')
>>> +        self.vm.event_wait(name='DEVICE_DELETED',
>>> +                           match={'data': {'device': 'net_4711'}})
>>> +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')
>>> +        exec_command_and_wait_for_pattern(self,
>>> +                                          'ls /sys/bus/ccw/devices/0.0.4711',
>>> +                                          'No such file or directory')



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-04 20:13     ` Wainer dos Santos Moschetta
@ 2020-12-07 11:12       ` Cornelia Huck
  0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2020-12-07 11:12 UTC (permalink / raw)
  To: Wainer dos Santos Moschetta
  Cc: Thomas Huth, qemu-devel, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Cleber Rosa, Philippe Mathieu-Daudé

On Fri, 4 Dec 2020 17:13:22 -0300
Wainer dos Santos Moschetta <wainersm@redhat.com> wrote:

> On 12/4/20 11:08 AM, Cornelia Huck wrote:
> > On Fri, 4 Dec 2020 11:05:34 -0300
> > Wainer dos Santos Moschetta <wainersm@redhat.com> wrote:
> >  
> >> Hi,
> >>
> >> On 12/4/20 9:14 AM, 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', ' ')  
> >>
> >> The problem is that `dmesg -c` will fail if you run the test with
> >> unprivileged user.  
> > Hm, why should that make a difference for a guest command?  
> 
> 
> Never mind, my brain mix host and guest very often....

I'm sure you're not the only one :)

> 
> Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>

Thanks!

> 
> 
> >  
> >> - Wainer
> >>  
> >>> +        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')
> >>> +        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', ' ')
> >>> +        self.vm.command('device_del', id='net_4711')
> >>> +        self.vm.event_wait(name='DEVICE_DELETED',
> >>> +                           match={'data': {'device': 'net_4711'}})
> >>> +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')
> >>> +        exec_command_and_wait_for_pattern(self,
> >>> +                                          'ls /sys/bus/ccw/devices/0.0.4711',
> >>> +                                          'No such file or directory')  



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-04 12:14 [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices Cornelia Huck
  2020-12-04 14:05 ` Wainer dos Santos Moschetta
@ 2020-12-07 14:28 ` Thomas Huth
  2020-12-07 16:30   ` Cornelia Huck
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2020-12-07 14:28 UTC (permalink / raw)
  To: Cornelia Huck, qemu-devel
  Cc: Wainer dos Santos Moschetta, Halil Pasic, Christian Borntraeger,
	qemu-s390x, Cleber Rosa, Philippe Mathieu-Daudé

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', '~ #')

> +        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)

> +        self.vm.command('device_del', id='net_4711')
> +        self.vm.event_wait(name='DEVICE_DELETED',
> +                           match={'data': {'device': 'net_4711'}})
> +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')

dito

> +        exec_command_and_wait_for_pattern(self,
> +                                          'ls /sys/bus/ccw/devices/0.0.4711',
> +                                          'No such file or directory')

With my suggestion applied:

Reviewed-by: Thomas Huth <thuth@redhat.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-07 14:28 ` Thomas Huth
@ 2020-12-07 16:30   ` Cornelia Huck
  2020-12-07 16:34     ` Thomas Huth
  0 siblings, 1 reply; 10+ messages in thread
From: Cornelia Huck @ 2020-12-07 16:30 UTC (permalink / raw)
  To: Thomas Huth
  Cc: qemu-devel, Wainer dos Santos Moschetta, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Cleber Rosa,
	Philippe Mathieu-Daudé

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?

> 
> > +        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.)

> 
> > +        self.vm.command('device_del', id='net_4711')
> > +        self.vm.event_wait(name='DEVICE_DELETED',
> > +                           match={'data': {'device': 'net_4711'}})
> > +        exec_command_and_wait_for_pattern(self, 'dmesg', 'CRW')  
> 
> dito
> 
> > +        exec_command_and_wait_for_pattern(self,
> > +                                          'ls /sys/bus/ccw/devices/0.0.4711',
> > +                                          'No such file or directory')  
> 
> With my suggestion applied:
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-07 16:30   ` Cornelia Huck
@ 2020-12-07 16:34     ` Thomas Huth
  2020-12-07 18:40       ` Thomas Huth
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2020-12-07 16:34 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: qemu-devel, Wainer dos Santos Moschetta, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Cleber Rosa,
	Philippe Mathieu-Daudé

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



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-07 16:34     ` Thomas Huth
@ 2020-12-07 18:40       ` Thomas Huth
  2020-12-08 11:43         ` Cornelia Huck
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Huth @ 2020-12-07 18:40 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: qemu-devel, Wainer dos Santos Moschetta, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Cleber Rosa,
	Philippe Mathieu-Daudé

On 07/12/2020 17.34, Thomas Huth wrote:
> 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>
>>>> ---
>
[...]
>>>> +        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.

Ok, as we had to discover during some testing, it's a bad idea to only wait
for ' ' after the 'dmesg -c' since it matches too early, so that the device
gets added while the dmesg command is still running.

The following code is working for me instead:

         # add another device
         exec_command_and_wait_for_pattern(self,
                     'dmesg -c > /dev/null; echo dm-clear\ 1', 'dm-clear 1')
         self.vm.command('device_add', driver='virtio-net-ccw',
                         devno='fe.0.4711', id='net_4711')
         exec_command_and_wait_for_pattern(self,
                         'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
                         'CRW reports')
         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 > /dev/null; echo dm-clear\ 2', 'dm-clear 2')
         self.vm.command('device_del', id='net_4711')
         self.vm.event_wait(name='DEVICE_DELETED',
                            match={'data': {'device': 'net_4711'}})
         exec_command_and_wait_for_pattern(self,
                         'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
                         'CRW reports')
         exec_command_and_wait_for_pattern(self,
                                           'ls /sys/bus/ccw/devices/0.0.4711',
                                           'No such file or directory')

 HTH,
  Thomas



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices
  2020-12-07 18:40       ` Thomas Huth
@ 2020-12-08 11:43         ` Cornelia Huck
  0 siblings, 0 replies; 10+ messages in thread
From: Cornelia Huck @ 2020-12-08 11:43 UTC (permalink / raw)
  To: Thomas Huth
  Cc: qemu-devel, Wainer dos Santos Moschetta, Halil Pasic,
	Christian Borntraeger, qemu-s390x, Cleber Rosa,
	Philippe Mathieu-Daudé

On Mon, 7 Dec 2020 19:40:36 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 07/12/2020 17.34, Thomas Huth wrote:
> > 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>
> >>>> ---  
> >  
> [...]
> >>>> +        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.  
> 
> Ok, as we had to discover during some testing, it's a bad idea to only wait
> for ' ' after the 'dmesg -c' since it matches too early, so that the device
> gets added while the dmesg command is still running.
> 
> The following code is working for me instead:
> 
>          # add another device
>          exec_command_and_wait_for_pattern(self,
>                      'dmesg -c > /dev/null; echo dm-clear\ 1', 'dm-clear 1')
>          self.vm.command('device_add', driver='virtio-net-ccw',
>                          devno='fe.0.4711', id='net_4711')
>          exec_command_and_wait_for_pattern(self,
>                          'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
>                          'CRW reports')
>          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 > /dev/null; echo dm-clear\ 2', 'dm-clear 2')
>          self.vm.command('device_del', id='net_4711')
>          self.vm.event_wait(name='DEVICE_DELETED',
>                             match={'data': {'device': 'net_4711'}})
>          exec_command_and_wait_for_pattern(self,
>                          'while ! (dmesg -c | grep CRW) ; do sleep 1 ; done',
>                          'CRW reports')
>          exec_command_and_wait_for_pattern(self,
>                                            'ls /sys/bus/ccw/devices/0.0.4711',
>                                            'No such file or directory')

Thanks for tracking this down, this works for me as well. I'll send a
v3.



^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-12-08 11:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04 12:14 [PATCH v2] tests/acceptance: test hot(un)plug of ccw devices Cornelia Huck
2020-12-04 14:05 ` Wainer dos Santos Moschetta
2020-12-04 14:08   ` Cornelia Huck
2020-12-04 20:13     ` Wainer dos Santos Moschetta
2020-12-07 11:12       ` Cornelia Huck
2020-12-07 14:28 ` Thomas Huth
2020-12-07 16:30   ` Cornelia Huck
2020-12-07 16:34     ` Thomas Huth
2020-12-07 18:40       ` Thomas Huth
2020-12-08 11:43         ` Cornelia Huck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).