[blktests,1/4] tests/nvme/rc: Modify the approach for disabling and re-enabling Ctrl-C
diff mbox series

Message ID 20190808200506.186137-2-bvanassche@acm.org
State New
Headers show
Series
  • Four blktests patches
Related show

Commit Message

Bart Van Assche Aug. 8, 2019, 8:05 p.m. UTC
Avoid that the following error messages are reported when redirecting stdin:

stty: 'standard input': Inappropriate ioctl for device
stty: 'standard input': Inappropriate ioctl for device

Cc: Logan Gunthorpe <logang@deltatee.com>
Cc: Johannes Thumshirn <jthumshirn@suse.de>
Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are removed on cleanup")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 tests/nvme/rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Logan Gunthorpe Aug. 8, 2019, 8:08 p.m. UTC | #1
On 2019-08-08 2:05 p.m., Bart Van Assche wrote:
> Avoid that the following error messages are reported when redirecting stdin:
> 
> stty: 'standard input': Inappropriate ioctl for device
> stty: 'standard input': Inappropriate ioctl for device
> 
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Cc: Johannes Thumshirn <jthumshirn@suse.de>
> Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are removed on cleanup")
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  tests/nvme/rc | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/nvme/rc b/tests/nvme/rc
> index d4e18e635dea..40f0413d32d2 100644
> --- a/tests/nvme/rc
> +++ b/tests/nvme/rc
> @@ -36,7 +36,7 @@ _cleanup_nvmet() {
>  	fi
>  
>  	# Don't let successive Ctrl-Cs interrupt the cleanup processes
> -	stty -isig
> +	trap '' SIGINT

Did you test this? Pretty sure I tried using trap and it didn't work,
probably because it's already running inside a trapped SIGINT.

Maybe it'd be better to just ignore any errors stty produces and pipe to
/dev/null?

Logan
Bart Van Assche Aug. 8, 2019, 9 p.m. UTC | #2
On 8/8/19 1:08 PM, Logan Gunthorpe wrote:
> On 2019-08-08 2:05 p.m., Bart Van Assche wrote:
>> Avoid that the following error messages are reported when redirecting stdin:
>>
>> stty: 'standard input': Inappropriate ioctl for device
>> stty: 'standard input': Inappropriate ioctl for device
>>
>> Cc: Logan Gunthorpe <logang@deltatee.com>
>> Cc: Johannes Thumshirn <jthumshirn@suse.de>
>> Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are removed on cleanup")
>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   tests/nvme/rc | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/nvme/rc b/tests/nvme/rc
>> index d4e18e635dea..40f0413d32d2 100644
>> --- a/tests/nvme/rc
>> +++ b/tests/nvme/rc
>> @@ -36,7 +36,7 @@ _cleanup_nvmet() {
>>   	fi
>>   
>>   	# Don't let successive Ctrl-Cs interrupt the cleanup processes
>> -	stty -isig
>> +	trap '' SIGINT
> 
> Did you test this? Pretty sure I tried using trap and it didn't work,
> probably because it's already running inside a trapped SIGINT.
> 
> Maybe it'd be better to just ignore any errors stty produces and pipe to
> /dev/null?

Hi Logan,

I don't think that redirecting the stty errors would be sufficient 
because Ctrl-C still works even if stdin, stdout and stderr are 
redirected. A command like sleep 60 </dev/null >&/dev/null can be 
interrupted with Ctrl-C but stty -isig >&/dev/null does not suppress 
Ctrl-C if stdin is redirected.

Are there other trap SIGINT statements in the blktests code? Does that 
mean that I overlooked something?

Thanks,

Bart.
Logan Gunthorpe Aug. 8, 2019, 9:11 p.m. UTC | #3
On 2019-08-08 3:00 p.m., Bart Van Assche wrote:
> On 8/8/19 1:08 PM, Logan Gunthorpe wrote:
>> On 2019-08-08 2:05 p.m., Bart Van Assche wrote:
>>> Avoid that the following error messages are reported when redirecting
>>> stdin:
>>>
>>> stty: 'standard input': Inappropriate ioctl for device
>>> stty: 'standard input': Inappropriate ioctl for device
>>>
>>> Cc: Logan Gunthorpe <logang@deltatee.com>
>>> Cc: Johannes Thumshirn <jthumshirn@suse.de>
>>> Fixes: a987b10bc179 ("nvme: Ensure all ports and subsystems are
>>> removed on cleanup")
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>>   tests/nvme/rc | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/nvme/rc b/tests/nvme/rc
>>> index d4e18e635dea..40f0413d32d2 100644
>>> --- a/tests/nvme/rc
>>> +++ b/tests/nvme/rc
>>> @@ -36,7 +36,7 @@ _cleanup_nvmet() {
>>>       fi
>>>         # Don't let successive Ctrl-Cs interrupt the cleanup processes
>>> -    stty -isig
>>> +    trap '' SIGINT
>>
>> Did you test this? Pretty sure I tried using trap and it didn't work,
>> probably because it's already running inside a trapped SIGINT.
>>
>> Maybe it'd be better to just ignore any errors stty produces and pipe to
>> /dev/null?
> 
> Hi Logan,
> 
> I don't think that redirecting the stty errors would be sufficient
> because Ctrl-C still works even if stdin, stdout and stderr are
> redirected. A command like sleep 60 </dev/null >&/dev/null can be
> interrupted with Ctrl-C but stty -isig >&/dev/null does not suppress
> Ctrl-C if stdin is redirected.

Ok, actually I just tested your change and it does work. I must have
done something slightly differently when I first tried it (I think I
added a handler which echoed messages which still let the SIGINTs
through to child processes but it works with the null string).

So:

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

> Are there other trap SIGINT statements in the blktests code? Does that
> mean that I overlooked something?

The main code traps EXIT which calls the cleanup handler that this trap
then overrides SIGINT with, so I'm not really sure how they interact.

Thanks,

Logan
Bart Van Assche Aug. 8, 2019, 9:18 p.m. UTC | #4
On 8/8/19 2:11 PM, Logan Gunthorpe wrote:
> The main code traps EXIT which calls the cleanup handler that this trap
> then overrides SIGINT with, so I'm not really sure how they interact.

Hi Logan,

You may want to know that there is already code in the blktests project 
that modifies a trap handler from inside a trap handler:

	trap 'trap "" EXIT; teardown' EXIT

Bart.

Patch
diff mbox series

diff --git a/tests/nvme/rc b/tests/nvme/rc
index d4e18e635dea..40f0413d32d2 100644
--- a/tests/nvme/rc
+++ b/tests/nvme/rc
@@ -36,7 +36,7 @@  _cleanup_nvmet() {
 	fi
 
 	# Don't let successive Ctrl-Cs interrupt the cleanup processes
-	stty -isig
+	trap '' SIGINT
 
 	shopt -s nullglob
 
@@ -66,7 +66,7 @@  _cleanup_nvmet() {
 	done
 
 	shopt -u nullglob
-	stty isig
+	trap SIGINT
 
 	modprobe -r nvme-loop 2>/dev/null
 	modprobe -r nvmet 2>/dev/null