Sorry, I missed most of this email before pushing the patch.
On 05/04/2010 06:22 PM, Denys Vlasenko wrote:
On Mon, 2010-05-03 at 21:11 +0200, Karel Klic wrote:
+ /* Log cli errors to stderr. */
+ logmode = LOGMODE_STDIO;
+
This is the default, no need to set it
Ok.
+static void set_echo(bool enabled)
+{
+ if (isatty(STDIN_FILENO) == 0)
+ {
+ /* Clean errno, which is set by isatty. */
+ errno = 0;
+ return;
+ }
+
+ struct termios t;
+ if (tcgetattr(STDIN_FILENO,&t)< 0)
+ perror_msg_and_die("tcgetattr failed");
This makes scripting (a-la "abrt-cli ...<file") nearly impossible.
If tcgetattr fails, then stdin is not a tty, don't panic, just return.
Ok.
+ set_echo(false);
read_from_stdin(_("Enter your password: "), result, 64);
+ set_echo(true);
It makes sense to _restore_ echo, not _enable_ - what if it was
already off? Make set_echo return old state:
old_echo = set_echo(false);
read_from_stdin(_("Enter your password: "), result, 64);
set_echo(old_echo);
The right solution seems to indicate if set_echo
changed the state of
echo. So if isatty() returns 0, tcgetattr() fails, or ECHO is already
turned off it returns FALSE for set_echo(false).
Thanks for your feedback. I'll commit the improvements now.
Karel