[Nut-upsdev] [PATCH] fix this spurious tcsetattr()/tcgetattr() change once and for all
Arnaud Quette
aquette.dev at gmail.com
Wed May 16 14:11:13 UTC 2012
2012/3/13 Greg A. Woods <woods at planix.com>:
> From: "Greg A. Woods" <woods at planix.com>
>
> The most likely cause of all spurious differences between what was set
> on the port with tcsetattr() and what tcgetattr() shows are likely to do
> with the c_local PENDIN flag, which is a status bit, not a control bit.
> It will change when there's unread pending input, which can be quite
> often on an APC UPS.
>
> The right way to compare struct termios values is to clear the status
> flags _after_ the tcsetattr() and of course after the tcgetattr() call
> and then compare the result with what was set.
>
> Also set NOKERNINFO, if available, as we don't want the UPS or noise on
> the line to accidentally trigger status output back to the UPS, and
> finally make sure IEXTEN is also cleared along with ISIG since it too
> can cause weird things to happen.
>
> This change also adds some debug code to show any differences in the
> structures in a logical manner in debug output (and squashes one tiny
> compiler warning).
> ---
> drivers/apcsmart.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 77 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c
> index 932dd9c..430368e 100644
> --- a/drivers/apcsmart.c
> +++ b/drivers/apcsmart.c
> @@ -188,7 +188,10 @@ static void apc_ser_set(void)
> tio.c_cflag |= (CS8 | CLOCAL | CREAD);
>
> tio.c_lflag |= ICANON;
> - tio.c_lflag &= ~ISIG;
> +#ifdef NOKERNINFO
> + tio.c_lflag |= NOKERNINFO;
> +#endif
> + tio.c_lflag &= ~(ISIG | IEXTEN);
>
> tio.c_iflag |= (IGNCR | IGNPAR);
> tio.c_iflag &= ~(IXON | IXOFF);
> @@ -220,12 +223,83 @@ static void apc_ser_set(void)
> if (tcsetattr(upsfd, TCSANOW, &tio))
> fatal_with_errno(EXIT_FAILURE, "tcsetattr(%s)", device_path);
>
> + /* clear status flags so that they don't affect our binary compare */
> +#ifdef PENDIN
> + tio.c_lflag &= ~PENDIN;
> +#endif
> +#ifdef FLUSHO
> + tio.c_lflag &= ~FLUSHO;
> +#endif
> +
> memset(&tio_chk, 0, sizeof(tio_chk));
> if (tcgetattr(upsfd, &tio_chk))
> fatal_with_errno(EXIT_FAILURE, "tcgetattr(%s)", device_path);
> - if (memcmp(&tio_chk, &tio, sizeof(tio)))
> +
> + /* clear status flags so that they don't affect our binary compare */
> +#ifdef PENDIN
> + tio_chk.c_lflag &= ~PENDIN;
> +#endif
> +#ifdef FLUSHO
> + tio_chk.c_lflag &= ~FLUSHO;
> +#endif
> +
> + if (memcmp(&tio_chk, &tio, sizeof(tio))) {
> + struct cchar {
> + const char *name;
> + int sub;
> + u_char def;
> + };
> + const struct cchar cchars1[] = {
> + { "discard", VDISCARD, CDISCARD },
> + { "dsusp", VDSUSP, CDSUSP },
> + { "eof", VEOF, CEOF },
> + { "eol", VEOL, CEOL },
> + { "eol2", VEOL2, CEOL },
> + { "erase", VERASE, CERASE },
> + { "intr", VINTR, CINTR },
> + { "kill", VKILL, CKILL },
> + { "lnext", VLNEXT, CLNEXT },
> + { "min", VMIN, CMIN },
> + { "quit", VQUIT, CQUIT },
> + { "reprint", VREPRINT, CREPRINT },
> + { "start", VSTART, CSTART },
> + { "status", VSTATUS, CSTATUS },
> + { "stop", VSTOP, CSTOP },
> + { "susp", VSUSP, CSUSP },
> + { "time", VTIME, CTIME },
> + { "werase", VWERASE, CWERASE },
> + { .name = NULL },
> + };
> + const struct cchar *cp;
> + struct termios *tp;
> +
> upslogx(LOG_NOTICE, "%s: device reports different attributes than what were set", device_path);
>
> + /*
> + * According to the manual the most common problem is
> + * mis-matched combinations of input and output baud rates. If
> + * the combinaion is not supported then neither are changed.
> + * This should not be a problem here since we set them both to
> + * the same extremely common rate of 2400.
> + */
> +
> + tp = &tio;
> + upsdebugx(1, "tcsetattr(): gfmt1:cflag=%x:iflag=%x:lflag=%x:oflag=%x:",
> + (unsigned int) tp->c_cflag, (unsigned int) tp->c_iflag,
> + (unsigned int) tp->c_lflag, (unsigned int) tp->c_oflag);
> + for (cp = cchars1; cp->name; ++cp)
> + upsdebugx(1, "\t%s=%x:", cp->name, tp->c_cc[cp->sub]);
> + upsdebugx(1, "\tispeed=%d:ospeed=%d", (int) cfgetispeed(tp), (int) cfgetospeed(tp));
> +
> + tp = &tio_chk;
> + upsdebugx(1, "tcgetattr(): gfmt1:cflag=%x:iflag=%x:lflag=%x:oflag=%x:",
> + (unsigned int) tp->c_cflag, (unsigned int) tp->c_iflag,
> + (unsigned int) tp->c_lflag, (unsigned int) tp->c_oflag);
> + for (cp = cchars1; cp->name; ++cp)
> + upsdebugx(1, "\t%s=%x:", cp->name, tp->c_cc[cp->sub]);
> + upsdebugx(1, "\tispeed=%d:ospeed=%d", (int) cfgetispeed(tp), (int) cfgetospeed(tp));
> + }
> +
> cable = getval("cable");
> if (cable && !strcasecmp(cable, ALT_CABLE_1)) {
> if (ser_set_dtr(upsfd, 1) == -1)
> @@ -265,7 +339,7 @@ static void ups_status_set(void)
>
> static void check_temperature(void)
> {
> - char *val;
> + const char *val;
> int maxtemp = APC_MELTING_TEMP;
>
> if ((val = getval("maxtemp"))) {
applied to the trunk, r3603, with some mods:
- I've only left the last (3rd) piece, which requires the "check
temperature" patch, postponed for now.
- VDSUSP and VSTATUS are not Posix compliant, nor supported on Linux.
I've thus added #ifdef {VSTATUS,VDSUSP} around each.
Could you please counter validate this?
- note that there were indent issues, Ie most (if not all) your
patches were space indented.
thanks.
Arnaud
--
Linux / Unix Expert R&D - Eaton - http://powerquality.eaton.com
Network UPS Tools (NUT) Project Leader - http://www.networkupstools.org/
Debian Developer - http://www.debian.org
Free Software Developer - http://arnaud.quette.free.fr/
More information about the Nut-upsdev
mailing list