Skip to contentSkip to conversation

Avoiding warnings when piping to sensors-detect

PATCH In review Jaromir Capik 2 participants 8 messages Jul 25, 2013 – Sep 11, 2013

lore
Share & tools
lore

Jaromir Capik

author
#0 lore raw

PATCH 1/1 Avoiding warnings when piping to sensors-detect

Hello.

The attached patch fixes <STDIN> handling when the user input is taken from /dev/null.
I know this way of generating the config is undocumented and therefore unsupported, but it seems people use it that way as the script doesn't support running in batch mode.

The patch was created for 3.3.4, but can be directly applied on the latest sources too.

Please, merge the fix.

Thanks in advance.

Regards,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / BaseOS

Email: [email protected]
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016

prog/detect/sensors-detect +3 -3
diff -Naur lm_sensors-3.3.4.orig/prog/detect/sensors-detect lm_sensors-3.3.4/prog/detect/sensors-detect
@@ -3707,7 +3707,7 @@
 	       "Do you want to scan it? (\%s/selectively): ",
 	       $default ? "YES/no" : "yes/NO";
 
-	$input = <STDIN>;
+	$input = <STDIN> || '';
 	if ($input =~ /^\s*n/i
 	 || (!$default && $input !~ /^\s*[ys]/i)) {
 		print "\n";

@@ -3718,7 +3718,7 @@
 		print "Please enter one or more addresses not to scan. Separate them with commas.\n",
 		      "You can specify a range by using dashes. Example: 0x58-0x5f,0x69.\n",
 		      "Addresses: ";
-		$input = <STDIN>;
+		$input = <STDIN> || '';
 		chomp($input);
 		@not_to_scan = parse_not_to_scan(0x03, 0x77, $input);
 	} elsif (($class & 0xff00) == 0x0300) {

@@ -6859,7 +6859,7 @@
 		       "safe though. Yes, you do have ISA I/O ports even if you do not have any\n".
 		       "ISA slots! Do you want to scan the ISA I/O ports? (\%s): ",
 		       $superio_features ? "yes/NO" : "YES/no";
-		$input = <STDIN>;
+		$input = <STDIN> || '';
 		unless ($input =~ /^\s*n/i
 		     || ($superio_features && $input !~ /^\s*y/i)) {
 			if (initialize_ioports()) {

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#1 lore raw

Hi Jaromir,

Quoted 11 lines · Jaromir Capik

The attached patch fixes <STDIN> handling when the user input is taken from /dev/null.
I know this way of generating the config is undocumented and therefore unsupported, but it seems people use it that way as the script doesn't support running in batch mode.

The patch was created for 3.3.4, but can be directly applied on the latest sources too.

Please, merge the fix.

Hmm, as far as I can see this solves the same problem as this patch of mine I posted some months ago:
http://marc.info/?l=lm-sensors&m5091022118747&w=2

The only reason why I did not commit it yet is that I was waiting for feedback, but the original requester never provided it :(

If my patch works for you and your users then I'll just apply it.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#2 lore raw
In reply to Jaromir Capik

Hi Jean.

Quoted 15 lines

Hi Jaromir,

Quoted 11 lines · Jaromir Capik

The attached patch fixes <STDIN> handling when the user input is taken from /dev/null.
I know this way of generating the config is undocumented and therefore unsupported, but it seems people use it that way as the script doesn't support running in batch mode.

The patch was created for 3.3.4, but can be directly applied on the latest sources too.

Please, merge the fix.

Quoted 8 lines

Hmm, as far as I can see this solves the same problem as this patch of mine I posted some months ago:
http://marc.info/?l=lm-sensors&m5091022118747&w=2

The only reason why I did not commit it yet is that I was waiting for feedback, but the original requester never provided it :(

If my patch works for you and your users then I'll just apply it.

I was unable to apply your patch directly, but I believe it must work correctly.
Having a switch is always better than piping. However, I know about users who already built their solution with redirection and that's why I'd like to propose one tiny modification of your patch in order to make it more robust for cases like that.

Please, teplace the following line:

	return <STDIN>;

with this:

	return <STDIN> || '';

This would prevent the script from printing warnings about uninitialised value. <STDIN> acts as uninitialised value when there are no more data on the standard input. The modified expression returns empty string instead.

Regardless of the fact, that users should not use it that way, it's more safe/correct.

Thanks in advance.

Regards,
Jaromir.

--
Jaromir Capik
Red Hat Czech, s.r.o.
Software Engineer / BaseOS

Email: [email protected]
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#3 lore raw
In reply to Jaromir Capik

Hi Jaromir,

Quoted 11 lines · Jaromir Capik
Quoted 8 lines

Hmm, as far as I can see this solves the same problem as this patch of mine I posted some months ago:
http://marc.info/?l=lm-sensors&m5091022118747&w=2

The only reason why I did not commit it yet is that I was waiting for feedback, but the original requester never provided it :(

If my patch works for you and your users then I'll just apply it.

I was unable to apply your patch directly, but
I believe it must work correctly.

Maybe the list archive corrupted it. I have made it available at:
http://khali.linux-fr.org/devel/lm-sensors/sensors-detect-implement-auto-mode.patch

Quoted 10 lines

Having a switch is always better than piping. However, I know about users who already built their solution with redirection and that's why I'd like to propose one tiny modification of your patch in order to make it more robust for cases like that.

Please, teplace the following line:

	return <STDIN>;
Quoted 8 lines

with this:

	return <STDIN> || '';

This would prevent the script from printing warnings about uninitialised value. <STDIN> acts as uninitialised value when there are no more data on the standard input. The modified expression returns empty string instead.

Yes, I understand that. However I'm not sure we want to encourage people to use this. --auto has the advantage that we could decide to change the behavior based on feedback, maybe adding different sub-options for different use cases or diverging from the interactive defaults. OTOH piping is out of our control and will just use the default answer to all interactive questions.

But I do agree that the warnings are ugly and should be avoided in all cases. My proposal would be to first detect when <STDIN> can't be read (eof(STDIN) should do) and bail out with an error message pointing the user to the new --auto parameter.

Would it be OK with you? Or do you prefer that we keep supporting the old way, even if it was never documented? If so, we could still print a warning but go on, and remove support for piping in a couple years, when everyone have updated their scripts.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#4 lore raw
In reply to Jaromir Capik
Quoted 1 line

Hi Jaromir,

Hi Jean

Quoted 22 lines
Quoted 6 lines

Please, teplace the following line:

	return <STDIN>;

with this:

Quoted 7 lines
	return <STDIN> || '';

This would prevent the script from printing warnings about uninitialised value. <STDIN> acts as uninitialised value when there are no more data on the standard input. The modified expression returns empty string instead.

Yes, I understand that. However I'm not sure we want to encourage people to use this.

But I do agree that the warnings are ugly and should be avoided in all cases. My proposal would be to first detect when <STDIN> can't be read (eof(STDIN) should do) and bail out with an error message pointing the user to the new --auto parameter.

Quoted 4 lines

Would it be OK with you? Or do you prefer that we keep supporting the old way, even if it was never documented? If so, we could still print a warning but go on, and remove support for piping in a couple years, when everyone have updated their scripts.

I prefer to keep support for the old way -AND- write a message instructing users to use the --auto switch instead. The message should be displayed at the bottom of the listing. Such way is the least invasive one. We'll not break the current solutions running with redirection while encouraging users to use the new way. Preventing users from using /dev/null as the input source would cause unwanted regressions in their current solutions and we don't want to make them angry, right? :]

Please, let me know about your decision.

Thank you.

Regards,
Jaromir.

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#5 lore raw
In reply to Jaromir Capik

Hi Jaromir,

Quoted 21 lines · Jaromir Capik
Quoted 8 lines

Yes, I understand that. However I'm not sure we want to encourage people to use this.

But I do agree that the warnings are ugly and should be avoided in all cases. My proposal would be to first detect when <STDIN> can't be read (eof(STDIN) should do) and bail out with an error message pointing the user to the new --auto parameter.

Quoted 4 lines

Would it be OK with you? Or do you prefer that we keep supporting the old way, even if it was never documented? If so, we could still print a warning but go on, and remove support for piping in a couple years, when everyone have updated their scripts.

I prefer to keep support for the old way -AND- write a message instructing users to use the --auto switch instead. The message should be displayed at the bottom of the listing. Such way is the least invasive one. We'll not break the current solutions running with redirection while encouraging users to use the new way. Preventing users from using /dev/null as the input source would cause unwanted regressions in their current solutions and we don't want to make them angry, right? :]

Quoted 1 line

Please, let me know about your decision.

Fair enough. eof() did not work (it blocked when STDIN was not redirected) but I found another way to implement your proposal in the Perl Cookbook. What do you think? If you can find a better wording, please let me know.

sensors-detect: Detect incorrect non-interactive runs

It is better to use option --auto for non-interactive runs of the sensors-detect script than faking an input to get the default answers to all questions.

---
 prog/detect/sensors-detect |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)
prog/detect/sensors-detect +8 -1
@@ -2518,7 +2518,7 @@ sub read_answer
 		print "\n";
 		return "";
 	}
-	return <STDIN>;
+	return <STDIN> || "";
 }
 
 ###################

@@ -7011,6 +7011,13 @@ sub main
 	}
 
 	unload_modules();
+
+	# Check if running non-interactively without --auto
+	if (!$opt_auto && ! -t STDIN) {
+		print "Warning: the preferred way to run this script non-interactively\n".
+		      "is with option --auto. Other methods are discouraged and may\n".
+		      "stop working at some point in the future.\n\n";
+	}
 }
 
 sub cleanup_on_int
Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
author
#6 lore raw
In reply to Jaromir Capik
Quoted 2 lines

Hi Jaromir,

Hi Jean.

Quoted 4 lines

Fair enough. eof() did not work (it blocked when STDIN was not redirected) but I found another way to implement your proposal in the Perl Cookbook. What do you think? If you can find a better wording, please let me know.

I believe the warning is explanatory enough. If it works correctly, then go on and push the patch.

Regards,
Jaromir.

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

#7 lore raw
In reply to Jaromir Capik
Quoted 7 lines · Jaromir Capik
Quoted 4 lines

Fair enough. eof() did not work (it blocked when STDIN was not redirected) but I found another way to implement your proposal in the Perl Cookbook. What do you think? If you can find a better wording, please let me know.

I believe the warning is explanatory enough. If it works correctly, then go on and push the patch.

Sorry for the delay, I just committed the patch.

Signature · 6 lines
Jean Delvare

_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors