[lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jaromir Capik]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

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: jcapik@redhat.com
Web: www.cz.redhat.com
Red Hat Czech s.r.o., Purkynova 99/71, 612 45, Brno, Czech Republic
IC: 27690016 



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lm_sensors-3.3.4-sensors-detect-null-input.patch --]
[-- Type: text/x-patch; name=lm_sensors-3.3.4-sensors-detect-null-input.patch, Size: 1307 bytes --]

diff -Naur lm_sensors-3.3.4.orig/prog/detect/sensors-detect lm_sensors-3.3.4/prog/detect/sensors-detect
--- lm_sensors-3.3.4.orig/prog/detect/sensors-detect	2013-05-20 21:25:22.000000000 +0200
+++ lm_sensors-3.3.4/prog/detect/sensors-detect	2013-07-25 18:24:41.636807410 +0200
@@ -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()) {

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jean Delvare]

Hi Jaromir,

On Thu, 25 Jul 2013 12:49:51 -0400 (EDT), Jaromir Capik wrote:
> 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&m\x135091022118747&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.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jaromir Capik]

Hi Jean.

> Hi Jaromir,
> 
> On Thu, 25 Jul 2013 12:49:51 -0400 (EDT), Jaromir Capik wrote:
> > 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&m\x135091022118747&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: jcapik@redhat.com
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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jean Delvare]

Hi Jaromir,

On Fri, 26 Jul 2013 06:01:30 -0400 (EDT), Jaromir Capik wrote:
> > 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&m\x135091022118747&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

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

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.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jaromir Capik]

> Hi Jaromir,

Hi Jean

> > 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.
> 
> 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.
> 
> 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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jean Delvare]

Hi Jaromir,

On Fri, 26 Jul 2013 10:31:02 -0400 (EDT), Jaromir Capik wrote:
> > 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.
> > 
> > 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.

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

--- lm-sensors.orig/prog/detect/sensors-detect	2013-07-27 17:46:45.193801963 +0200
+++ lm-sensors/prog/detect/sensors-detect	2013-07-28 14:12:02.481430020 +0200
@@ -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


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jaromir Capik]

> 
> Hi Jaromir,

Hi Jean.

> 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
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH] Avoiding warnings when piping to sensors-detect

[Jean Delvare]

On Tue, 30 Jul 2013 13:50:28 -0400 (EDT), Jaromir Capik wrote:
> > 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.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors