From 5225aeb56e255a9a7ab1841433b47aee0bdf2871 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 01:26:15 +0200 Subject: [PATCH 01/15] drivers/apcsmart.c: revise comments and log messages Signed-off-by: Jim Klimov --- drivers/apcsmart.c | 91 +++++++++++++++++++++++----------------------- 1 file changed, 46 insertions(+), 45 deletions(-) diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index bff3fee0ad..5d31d235eb 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -87,7 +87,7 @@ static int (*sdlist[])(const void *) = { /* * note: both lookup functions MUST be used after variable detection is * completed - that is after deprecate_vars() call; the general reason for this - * is 1:n and n:1 nut <-> apc mappings, which are not determined prior to the + * is 1:n and n:1 NUT <-> APC mappings, which are not determined prior to the * detection */ static apc_vartab_t *vt_lookup_char(char cmdchar) @@ -207,7 +207,7 @@ static const char *convert_data(apc_vartab_t *vt, const char *upsval) /* report differences if tcsetattr != tcgetattr, return otherwise */ /* - * Aix compatible names + * AIX compatible names */ #if defined(VWERSE) && !defined(VWERASE) #define VWERASE VWERSE @@ -276,7 +276,7 @@ static void apc_ser_diff(struct termios *tioset, struct termios *tioget) upslogx(LOG_NOTICE, "%s: device reports different attributes than requested", device_path); /* - * According to the manual the most common problem is mis-matched + * According to the manual the most common problem is mismatched * combinations of input and output baud rates. If the combination is * not supported then neither are changed. This should not be a * problem here since we set them both to the same extremely common @@ -441,7 +441,7 @@ static void alert_handler(char ch) } /* - * we need a tiny bit different processing due to '*' and canonical mode; the + * we need a tiny bit different processing due to "*" and canonical mode; the * function is subtly different from generic ser_get_line_alert() */ #define apc_read(b, l, f) apc_read_i(b, l, f, __func__, __LINE__) @@ -506,10 +506,10 @@ static ssize_t apc_read_i(char *buf, size_t buflen, int flags, const char *fn, u ser_comm_fail("serial port read timeout: %u(%s)", ln, fn); return ret; } - /* ok, timeout is acceptable */ + /* ok, timeout is acceptable... */ if (ret == 0 && (flags & SER_TO)) { /* - * but it doesn't imply ser_comm_good + * ...but it doesn't imply ser_comm_good * * for example we might be in comm_fail condition, * trying to "nudge" the UPS with some command @@ -535,17 +535,18 @@ static ssize_t apc_read_i(char *buf, size_t buflen, int flags, const char *fn, u return (ssize_t)count; } /* - * '*' is set as a secondary EOL; convert to 'OK' only as a - * reply to shutdown command in sdok(); otherwise next - * select_read() will continue normally + * "*" is set as a secondary EOL; convert to "OK" only + * as a reply to shutdown command in sdok(); otherwise + * next select_read() will continue normally */ if ((flags & SER_HA) && temp[i] == '*') { /* - * a bit paranoid, but remember '*' is not real EOL; - * there could be some firmware in existence, that - * would send both string: 'OK\n' and alert: '*'. - * Just in case, try to flush the input with small 1 sec. - * timeout + * a bit paranoid, but remember that "*" is not + * real EOL; there could be some firmware in + * existence, which would send both string: + * "OK\n" and alert: "*". + * Just in case, try to flush the input with a + * small 1 sec timeout. */ memset(buf, '\0', buflen); errno = 0; @@ -589,9 +590,9 @@ static ssize_t apc_write_i(unsigned char code, const char *fn, unsigned int ln) ret = ser_send_char(upsfd, code); /* - * Formally any write() sould never return 0, if the count != 0. For + * Formally any write() should never return 0, if the count != 0. For * the sake of handling any obscure nonsense, we consider such return - * as a failure - thus <= condition; either way, LE is pretty hard + * as a failure - thus <= condition; either way, LE is a pretty hard * condition hardly ever happening; */ if (ret <= 0) @@ -603,7 +604,7 @@ static ssize_t apc_write_i(unsigned char code, const char *fn, unsigned int ln) /* * We have to watch out for NA, here; * This is generally safe, as otherwise we will just timeout. The reason we do - * it, is that under certain conditions an ups might respond with NA for + * it, is that under certain conditions an UPS might respond with NA for * something it would normally handle (e.g. calling @ while being in powered * off or hibernated state. If we keep sending the "arguments" after getting * NA, they will be interpreted as commands, which is quite a bug :) @@ -677,7 +678,7 @@ static void apc_flush(int flags) } } -/* apc specific wrappers around set/del info - to handle "packed" variables */ +/* APC specific wrappers around set/del info - to handle "packed" variables */ static void apc_dstate_delinfo(apc_vartab_t *vt, int skip) { char *name, *nidx; @@ -905,10 +906,10 @@ static int var_verify(apc_vartab_t *vt) } /* - * This function iterates over vartab, deprecating nut<->apc 1:n and n:1 + * This function iterates over vartab, deprecating NUT<->APC 1:n and n:1 * variables. We prefer earliest present variable. All the other ones must be * marked as not present (which implies deprecation). - * This pass is requried after completion of all protocol_verify() and/or + * This pass is required after completion of all protocol_verify() and/or * legacy_verify() calls. */ static void deprecate_vars(void) @@ -978,7 +979,7 @@ static void apc_getcaps(int qco) /* * note - apc_read() needs larger timeout grace (not a problem w.r.t. - * to nut's timing, as it's done only during setup) and different + * to NUT's timing, as it's done only during setup) and different * ignore set due to certain characters like '#' being received */ ret = apc_read(temp, sizeof(temp), SER_CC|SER_TO); @@ -1118,7 +1119,7 @@ static void protocol_verify(unsigned char cmd) return; /* - * loop necessary for apc:nut 1:n cases (e.g. T -> device.uptime, + * loop necessary for APC:NUT 1:n cases (e.g. T -> device.uptime, * ambient.0.temperature) */ found = 0; @@ -1134,7 +1135,7 @@ static void protocol_verify(unsigned char cmd) /* * see if it's a command - * loop necessary for apc:nut 1:n cases (e.g. D -> calibrate.start, + * loop necessary for APC:NUT 1:n cases (e.g. D -> calibrate.start, * calibrate.stop) */ found = 0; @@ -1162,7 +1163,7 @@ static void oldapcsetup(void) { /* * note: battery.date and ups.id make little sense here, as - * that would imply writability and this is an *old* apc psu + * that would imply writability and this is an *old* APC UPS */ legacy_verify("ups.temperature"); legacy_verify("ups.load"); @@ -1183,19 +1184,19 @@ static void oldapcsetup(void) if (vt_lookup_name("output.current")) dstate_setinfo("ups.model", "Matrix-UPS"); else { - /* really old models don't support ups.model (apc: 0x01) */ + /* really old models don't support ups.model (APC: 0x01) */ if (!vt_lookup_name("ups.model")) /* force the model name */ dstate_setinfo("ups.model", "Smart-UPS"); } /* - * If we have come down this path then we dont do capabilities and + * If we have come down this path then we don't do capabilities and * other shiny features. */ } -/* some hardware is a special case - hotwire the list of cmdchars */ +/* some hardware is a special case - hot-wire the list of cmdchars */ static int firmware_table_lookup(void) { ssize_t ret; @@ -1345,7 +1346,7 @@ static int do_cal(int start) /* if we can't check the current calibration status, bail out */ if ((ret < 1) || (!strcmp(temp, "NA"))) { - upslogx(LOG_WARNING, "%s", "runtime calibration state undeterminable"); + upslogx(LOG_WARNING, "%s", "runtime calibration state can not be determined"); return STAT_INSTCMD_HANDLED; /* FUTURE: failure */ } @@ -1429,7 +1430,7 @@ static int smartmode(void) /* * get the UPS talking to us in smart mode * note: this is weird overkill, but possibly excused due to some obscure - * hardware/firmware combinations; simpler version commmented out above, for + * hardware/firmware combinations; simpler version commented out above, for * now let's keep minimally adjusted old one */ static int smartmode(int cnt) @@ -1444,7 +1445,7 @@ static int smartmode(int cnt) if (apc_write(APC_GOSMART) != 1) return 0; - /* timeout here is intented */ + /* timeout here is intended */ ret = apc_read(temp, sizeof(temp), SER_TO|SER_D1); if (ret > 0 && !strcmp(temp, "SM")) return 1; /* success */ @@ -1463,8 +1464,8 @@ static int smartmode(int cnt) } /* - * all shutdown commands should respond with 'OK' or '*' - * apc_read() handles conversion to 'OK' so we care only about that one + * all shutdown commands should respond with "OK" or "*" + * apc_read() handles conversion to "OK" so we care only about that one * ign allows for timeout without assuming an error */ static int sdok(int ign) @@ -1473,7 +1474,7 @@ static int sdok(int ign) char temp[APC_SBUF]; /* - * older upses on failed commands might just timeout, we cut down + * older UPSes on failed commands might just timeout, we cut down * timeout grace though * furthermore, command 'Z' will not reply with anything */ @@ -1563,7 +1564,7 @@ static int sdcmd_AT(const void *str) } strcpy(ptr, awd); - upsdebugx(1, "%s: issuing [%s] with %ld minutes of additional wakeup delay", + upsdebugx(1, "%s: issuing [%s] with %ld minutes of additional wake-up delay", __func__, prtchr(APC_CMD_GRACEDOWN), strtol(awd, NULL, 10)*6); apc_flush(0); @@ -1586,7 +1587,7 @@ static int sdcmd_AT(const void *str) /* * "tricky" part - we tried @nn variation and it (unsurprisingly) * failed; we have to abort the sequence with something bogus to have - * the clean state; newer upses will respond with 'NO', older will be + * the clean state; newer UPSes will respond with "NO", older will be * silent (YMMV); */ apc_write(APC_GOSMART); @@ -1626,7 +1627,7 @@ static int sdcmd_Z(const void *foo) return STAT_INSTCMD_FAILED; } - /* note: ups will not reply anything after this command */ + /* note: UPS will not reply anything after this command */ return sdok(1); } @@ -1777,7 +1778,7 @@ static int setvar_enum(apc_vartab_t *vt, const char *val) ptr = convert_data(vt, orig); - /* suppress redundant changes - easier on the eeprom */ + /* suppress redundant changes - easier on the EEPROM */ if (!strcmp(ptr, val)) { upslogx(LOG_INFO, "%s: ignoring SET %s='%s' (unchanged value)", __func__, vt->name, val); @@ -1860,7 +1861,7 @@ static int setvar_string(apc_vartab_t *vt, const char *val) if ((ret < 1) || (!strcmp(temp, "NA"))) return STAT_SET_FAILED; - /* suppress redundant changes - easier on the eeprom */ + /* suppress redundant changes - easier on the EEPROM */ if (!strcmp(temp, val)) { upslogx(LOG_INFO, "%s: ignoring SET %s='%s' (unchanged value)", __func__, vt->name, val); @@ -1933,7 +1934,7 @@ static int do_loadon(void) return STAT_INSTCMD_FAILED; /* - * ups will not reply anything after this command, but might + * UPS will not reply anything after this command, but might * generate brief OVER condition (which will be corrected on * the next status update) */ @@ -1942,7 +1943,7 @@ static int do_loadon(void) return STAT_INSTCMD_HANDLED; } -/* actually send the instcmd's char to the ups */ +/* actually send the instcmd's char to the UPS */ static int do_cmd(const apc_cmdtab_t *ct) { ssize_t ret; @@ -2085,7 +2086,7 @@ void upsdrv_makevartable(void) { addvar(VAR_VALUE, "ttymode", "tty discipline selection"); addvar(VAR_VALUE, "cable", "alternate cable (940-0095B) selection"); - addvar(VAR_VALUE, "awd", "hard hibernate's additional wakeup delay"); + addvar(VAR_VALUE, "awd", "hard hibernate's additional wake-up delay"); addvar(VAR_VALUE, "sdtype", "simple shutdown method"); addvar(VAR_VALUE, "advorder", "advanced shutdown control"); addvar(VAR_VALUE, "cshdelay", "CS hack delay"); @@ -2105,7 +2106,7 @@ void upsdrv_initups(void) char *val; apc_vartab_t *ptr; - /* sanitize awd (additional waekup delay of '@' command) */ + /* sanitize awd (additional wake-up delay of '@' command) */ if ((val = getval("awd")) && !rexhlp(APC_AWDFMT, val)) { fatalx(EXIT_FAILURE, "invalid value (%s) for option 'awd'", val); } @@ -2164,7 +2165,7 @@ void upsdrv_initinfo(void) ); } - /* manufacturer ID - hardcoded in this particular module */ + /* manufacturer ID - hard-coded in this particular module */ dstate_setinfo("ups.mfr", "APC"); if (!(pmod = dstate_getinfo("ups.model"))) @@ -2189,7 +2190,7 @@ void upsdrv_updateinfo(void) int all; time_t now; - /* try to wake up a dead ups once in awhile */ + /* try to wake up a dead UPS once in awhile */ if (dstate_is_stale()) { if (!last_worked) upsdebugx(1, "%s: %s", __func__, "comm lost"); @@ -2201,7 +2202,7 @@ void upsdrv_updateinfo(void) return; /* become aggressive after a few tries */ - upsdebugx(1, "%s: nudging ups with 'Y', iteration #%d ...", __func__, last_worked); + upsdebugx(1, "%s: nudging UPS with 'Y', iteration #%d ...", __func__, last_worked); if (!smartmode(1)) return; From f9edff843e41fe1510ed74692ba864053ee4e1d6 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 01:26:41 +0200 Subject: [PATCH 02/15] drivers/apcsmart.c: constrain strcpy() => strncpy() NOTE: Further improvement is possible in that strncpy() does not guarantee a NUL byte in the end of resulting string (if input is too long, it is only truncated). Signed-off-by: Jim Klimov --- NEWS.adoc | 4 ++++ docs/nut.dict | 3 ++- drivers/apcsmart.c | 46 ++++++++++++++++++++++++++++++++++------------ 3 files changed, 40 insertions(+), 13 deletions(-) diff --git a/NEWS.adoc b/NEWS.adoc index adc0e84c5e..9cbee8838b 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -98,6 +98,10 @@ https://github.com/networkupstools/nut/milestone/11 (similar to `runtimecal` in some other drivers, may be refactored to that configuration and logic model in later NUT releases) + - apcsmart updates: + * Revised code to use `strncpy()` and avoid potential overflows that are + possible with `strcpy()` used before. [PR #2564] + - added Visench C1K (using serial port converter with USB ID `1a86:7523`) as known supported by `nutdrv_qx` (Megatec protocol) since at least NUT v2.7.4 release [#2395] diff --git a/docs/nut.dict b/docs/nut.dict index 0fd8a7a9c8..70a570b866 100644 --- a/docs/nut.dict +++ b/docs/nut.dict @@ -1,4 +1,4 @@ -personal_ws-1.1 en 3199 utf-8 +personal_ws-1.1 en 3200 utf-8 AAC AAS ABI @@ -2845,6 +2845,7 @@ strerror strftime stringify strlen +strncpy strnlen strptime struct diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index 5d31d235eb..11e90f03fd 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -4,6 +4,7 @@ * Copyright (C) 1999 Russell Kroll * (C) 2000 Nigel Metheringham * (C) 2011+ Michal Soltys + * (C) 2024 Jim Klimov * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -36,7 +37,7 @@ #include "apcsmart_tabs.h" #define DRIVER_NAME "APC Smart protocol driver" -#define DRIVER_VERSION "3.33" +#define DRIVER_VERSION "3.34" #ifdef WIN32 # ifndef ECANCELED @@ -148,6 +149,8 @@ static const char *convert_data(apc_vartab_t *vt, const char *upsval) static char temp[APC_LBUF]; long tval; + memset(temp, '\0', sizeof(temp)); + /* this should never happen */ if (strlen(upsval) >= sizeof(temp)) { upslogx(LOG_CRIT, "%s: the length of [%s] is too big", __func__, vt->name); @@ -166,7 +169,8 @@ static const char *convert_data(apc_vartab_t *vt, const char *upsval) case APC_F_SECONDS: case APC_F_LEAVE: /* no conversion for any of these */ - strcpy(temp, upsval); + strncpy(temp, upsval, sizeof(temp) - 1); + temp[sizeof(temp) - 1] = '\0'; return temp; case APC_F_HOURS: @@ -193,14 +197,16 @@ static const char *convert_data(apc_vartab_t *vt, const char *upsval) case 'O': return "no transfers yet since turnon"; case 'S': return "simulated power failure or UPS test"; default: - strcpy(temp, upsval); - return temp; + strncpy(temp, upsval, sizeof(temp) - 1); + temp[sizeof(temp) - 1] = '\0'; + return temp; } } /* this should never happen */ upslogx(LOG_CRIT, "%s: unable to convert [%s]", __func__, vt->name); - strcpy(temp, upsval); + strncpy(temp, upsval, sizeof(temp) - 1); + temp[sizeof(temp) - 1] = '\0'; return temp; } @@ -695,7 +701,9 @@ static void apc_dstate_delinfo(apc_vartab_t *vt, int skip) return; } - strcpy(name, vt->name); + memset(name, '\0', vt->nlen0); + strncpy(name, vt->name, vt->nlen0 - 1); + name[vt->nlen0 - 1] = '\0'; nidx = strstr(name,".0.") + 1; for (c = skip; c < vt->cnt; c++) { @@ -711,6 +719,7 @@ static void apc_dstate_setinfo(apc_vartab_t *vt, const char *upsval) { char *name, *nidx; char *temp, *vidx[APC_PACK_MAX], *com, *curr; + size_t upsvallen = strlen(upsval); int c; /* standard not packed var */ @@ -724,18 +733,23 @@ static void apc_dstate_setinfo(apc_vartab_t *vt, const char *upsval) return; } - if ( !(temp = xmalloc(sizeof(char) * (strlen(upsval) + 1))) ) { + if ( !(temp = xmalloc(sizeof(char) * (upsvallen + 2))) ) { + /* +2 seems like an overkill, but helps hush compiler warnings */ upslogx(LOG_ERR, "apc_dstate_setinfo() failed to allocate buffer"); free(name); return; } /* we have to set proper name for dstate_setinfo() calls */ - strcpy(name, vt->name); + memset(name, '\0', vt->nlen0); + strncpy(name, vt->name, vt->nlen0 - 1); + name[vt->nlen0 - 1] = '\0'; nidx = strstr(name,".0.") + 1; /* split the value string */ - strcpy(temp, upsval); + memset(temp, '\0', upsvallen + 2); + strncpy(temp, upsval, upsvallen + 1); + temp[upsvallen] = '\0'; curr = temp; c = 0; do { @@ -1239,7 +1253,9 @@ static int firmware_table_lookup(void) * through 'b'; see: * http://article.gmane.org/gmane.comp.monitoring.nut.user/7762 */ - strcpy(buf, "set\1"); + memset(buf, '\0', sizeof(buf)); + strncpy(buf, "set\1", sizeof(buf) - 1); + buf[sizeof(buf) - 1] = '\0'; } for (i = 0; apc_compattab[i].firmware != NULL; i++) { @@ -1551,6 +1567,8 @@ static int sdcmd_AT(const void *str) const char *awd = str; char temp[APC_SBUF], *ptr; + memset(temp, '\0', sizeof(temp)); + if (!awd) awd = "000"; @@ -1562,7 +1580,8 @@ static int sdcmd_AT(const void *str) for (i = cnt; i < padto ; i++) { *ptr++ = '0'; } - strcpy(ptr, awd); + strncpy(ptr, awd, sizeof(temp) - (ptr - temp) - 1); + temp[sizeof(temp) - 1] = '\0'; upsdebugx(1, "%s: issuing [%s] with %ld minutes of additional wake-up delay", __func__, prtchr(APC_CMD_GRACEDOWN), strtol(awd, NULL, 10)*6); @@ -1852,6 +1871,8 @@ static int setvar_string(apc_vartab_t *vt, const char *val) return STAT_SET_FAILED; } + memset(temp, '\0', sizeof(temp)); + apc_flush(SER_AA); if (apc_write((const unsigned char)vt->cmd) != 1) return STAT_SET_FAILED; @@ -1871,7 +1892,8 @@ static int setvar_string(apc_vartab_t *vt, const char *val) /* length sanitized above */ temp[0] = APC_NEXTVAL; - strcpy(temp + 1, val); + strncpy(temp + 1, val, sizeof(temp) - 1); + temp[sizeof(temp) - 1] = '\0'; ptr = temp + strlen(temp); for (i = strlen(val); i < APC_STRLEN; i++) *ptr++ = '\015'; /* pad with CRs */ From 1ee54d6106a350e0ec727de9f740cd42aa299c40 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 01:46:45 +0200 Subject: [PATCH 03/15] drivers/apcsmart.c: update upsdebugx() to report __func__ where they did not Signed-off-by: Jim Klimov --- drivers/apcsmart.c | 58 ++++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 20 deletions(-) diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index 11e90f03fd..b608df4cef 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -290,12 +290,18 @@ static void apc_ser_diff(struct termios *tioset, struct termios *tioget) */ for (i = 0; i < SIZEOF_ARRAY(tio); i++) { - upsdebugx(1, "tc%cetattr(): gfmt1:cflag=%x:iflag=%x:lflag=%x:oflag=%x:", dir[i], - (unsigned int) tio[i]->c_cflag, (unsigned int) tio[i]->c_iflag, - (unsigned int) tio[i]->c_lflag, (unsigned int) tio[i]->c_oflag); + upsdebugx(1, + "%s: tc%cetattr(): gfmt1:cflag=%x:iflag=%x:lflag=%x:oflag=%x:", + __func__, dir[i], + (unsigned int) tio[i]->c_cflag, (unsigned int) tio[i]->c_iflag, + (unsigned int) tio[i]->c_lflag, (unsigned int) tio[i]->c_oflag); for (cp = cchars1; cp->name; ++cp) - upsdebugx(1, "\t%s=%x:", cp->name, tio[i]->c_cc[cp->sub]); - upsdebugx(1, "\tispeed=%d:ospeed=%d", (int) cfgetispeed(tio[i]), (int) cfgetospeed(tio[i])); + upsdebugx(1, "%s: \t%s=%x:", + __func__, cp->name, tio[i]->c_cc[cp->sub]); + upsdebugx(1, "%s: \tispeed=%d:ospeed=%d", + __func__, + (int) cfgetispeed(tio[i]), + (int) cfgetospeed(tio[i])); } } @@ -1016,7 +1022,8 @@ static void apc_getcaps(int qco) if (temp[0] != '#') { upslogx(LOG_WARNING, "unknown capability start char [%c] !", temp[0]); - upsdebugx(1, "please report this caps string: %s", temp); + upsdebugx(1, "%s: please report this caps string: %s", + __func__, temp); return; } @@ -1075,7 +1082,8 @@ static void apc_getcaps(int qco) /* mark this as writable */ if (valid) { - upsdebugx(1, "%s [%s(%c)] - capability supported", vt->name, prtchr(cmd), loc); + upsdebugx(1, "%s: %s [%s(%c)] - capability supported", + __func__, vt->name, prtchr(cmd), loc); dstate_setflags(vt->name, ST_FLAG_RW); @@ -1217,7 +1225,8 @@ static int firmware_table_lookup(void) unsigned int i, j; char buf[APC_LBUF]; - upsdebugx(1, "attempting firmware lookup using [%s]", prtchr(APC_FW_OLD)); + upsdebugx(1, "%s: attempting firmware lookup using [%s]", + __func__, prtchr(APC_FW_OLD)); apc_flush(0); if (apc_write(APC_FW_OLD) != 1) @@ -1230,7 +1239,8 @@ static int firmware_table_lookup(void) * firmware version, we attempt that only if 'V' doesn't work. */ if (!ret || !strcmp(buf, "NA")) { - upsdebugx(1, "attempting firmware lookup using [%s]", prtchr(APC_FW_NEW)); + upsdebugx(1, "%s: attempting firmware lookup using [%s]", + __func__, prtchr(APC_FW_NEW)); if (apc_write(APC_FW_NEW) != 1) return 0; @@ -1238,12 +1248,12 @@ static int firmware_table_lookup(void) return 0; } - upsdebugx(1, "detected firmware version: %s", buf); + upsdebugx(1, "%s: detected firmware version: %s", __func__, buf); /* this will be reworked if we get a lot of these things */ if (!strcmp(buf, "451.2.I")) { /* quirk_capability_overflow */ - upsdebugx(1, "WARN: quirky firmware !"); + upsdebugx(1, "%s: WARN: quirky firmware !", __func__); return 2; } @@ -1261,7 +1271,8 @@ static int firmware_table_lookup(void) for (i = 0; apc_compattab[i].firmware != NULL; i++) { if (!strcmp(apc_compattab[i].firmware, buf)) { - upsdebugx(1, "matched firmware: %s", apc_compattab[i].firmware); + upsdebugx(1, "%s: matched firmware: %s", + __func__, apc_compattab[i].firmware); /* magic ? */ if (strspn(apc_compattab[i].firmware, "05")) { @@ -1271,7 +1282,9 @@ static int firmware_table_lookup(void) } /* matched - run the cmdchars from the table */ - upsdebugx(1, "parsing out supported cmds and vars"); + upsdebugx(1, + "%s: parsing out supported cmds and vars", + __func__); for (j = 0; j < strlen(apc_compattab[i].cmdchars); j++) protocol_verify((const unsigned char)(apc_compattab[i].cmdchars[j])); deprecate_vars(); @@ -1299,7 +1312,8 @@ static int getbaseinfo(void) /* found compat */ return 1; - upsdebugx(1, "attempting var/cmdset lookup using [%s]", prtchr(APC_CMDSET)); + upsdebugx(1, "%s: attempting var/cmdset lookup using [%s]", + __func__, prtchr(APC_CMDSET)); /* * Initially we ask the UPS what commands it takes. If this fails we are * going to need an alternate strategy - we can deal with that if it @@ -1319,7 +1333,7 @@ static int getbaseinfo(void) return 1; } - upsdebugx(1, "parsing out supported cmds/vars"); + upsdebugx(1, "%s: parsing out supported cmds/vars", __func__); /* * returned set is verified for validity above, so just extract * what's interesting for us @@ -1338,7 +1352,7 @@ static int getbaseinfo(void) /* if capabilities are supported, add them here */ if (strchr(cmds, APC_CAPS)) { - upsdebugx(1, "parsing out caps"); + upsdebugx(1, "%s: parsing out caps", __func__); apc_getcaps(qco); } return 1; @@ -1536,7 +1550,9 @@ static int sdcmd_CS(const void *foo) if ((val = getval("cshdelay"))) cshd = (strtod(val, NULL) * 1000000); - upsdebugx(1, "%s: issuing CS 'hack' [%s+%s] with %2.1f sec delay", __func__, prtchr(APC_CMD_SIMPWF), prtchr(APC_CMD_SOFTDOWN), (double)cshd / 1000000); + upsdebugx(1, "%s: issuing CS 'hack' [%s+%s] with %2.1f sec delay", + __func__, prtchr(APC_CMD_SIMPWF), prtchr(APC_CMD_SOFTDOWN), + (double)cshd / 1000000); if (ups_status & APC_STAT_OL) { apc_flush(0); upsdebugx(1, "%s: issuing [%s]", __func__, prtchr(APC_CMD_SIMPWF)); @@ -1584,7 +1600,7 @@ static int sdcmd_AT(const void *str) temp[sizeof(temp) - 1] = '\0'; upsdebugx(1, "%s: issuing [%s] with %ld minutes of additional wake-up delay", - __func__, prtchr(APC_CMD_GRACEDOWN), strtol(awd, NULL, 10)*6); + __func__, prtchr(APC_CMD_GRACEDOWN), strtol(awd, NULL, 10)*6); apc_flush(0); ret = apc_write_long(temp); @@ -2195,7 +2211,8 @@ void upsdrv_initinfo(void) if (!(pser = dstate_getinfo("ups.serial"))) pser = "unknown serial"; - upsdebugx(1, "detected %s [%s] on %s", pmod, pser, device_path); + upsdebugx(1, "%s: detected %s [%s] on %s", + __func__, pmod, pser, device_path); setuphandlers(); /* @@ -2224,7 +2241,8 @@ void upsdrv_updateinfo(void) return; /* become aggressive after a few tries */ - upsdebugx(1, "%s: nudging UPS with 'Y', iteration #%d ...", __func__, last_worked); + upsdebugx(1, "%s: nudging UPS with 'Y', iteration #%d ...", + __func__, last_worked); if (!smartmode(1)) return; From b1cd1dc904a1b2abf610e9a560efabc9f6dd2ef7 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 03:01:57 +0200 Subject: [PATCH 04/15] drivers/apcsmart.c: upsdrv_updateinfo(): if dstate_is_stale, retry slowly [#704] Signed-off-by: Jim Klimov --- drivers/apcsmart.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index b608df4cef..125db7d885 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -2237,6 +2237,9 @@ void upsdrv_updateinfo(void) /* reset this so a full update runs when the UPS returns */ last_full = 0; + /* Do not flood the device (and our logs) with retry attempts */ + usleep(1000000); + if (++last_worked < 10) return; From b04697d4641bb5db8e5940b442dac31d866d3b4b Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 03:08:19 +0200 Subject: [PATCH 05/15] drivers/apcsmart.c: upsdrv_updateinfo(): if dstate_is_stale, try to flush the incoming data buffer [#704] Signed-off-by: Jim Klimov --- drivers/apcsmart.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index 125db7d885..d4392e10d6 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -2231,14 +2231,27 @@ void upsdrv_updateinfo(void) /* try to wake up a dead UPS once in awhile */ if (dstate_is_stale()) { + char temp[LARGEBUF]; + size_t count = 0; + if (!last_worked) upsdebugx(1, "%s: %s", __func__, "comm lost"); /* reset this so a full update runs when the UPS returns */ last_full = 0; - /* Do not flood the device (and our logs) with retry attempts */ - usleep(1000000); + /* Flush the buffer in case it helps, + * or sleep 1 sec if buffer is empty */ + while (select_read(upsfd, temp, sizeof(temp), 1, 0) > 0) { + count++; + } + errno = 0; + + if (!count) { + /* Do not flood the device (and our logs, below) + * with retry attempts */ + usleep(1000000); + } if (++last_worked < 10) return; From b36b86695d9b657067f2a113dcf6751c76904822 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 03:18:56 +0200 Subject: [PATCH 06/15] drivers/apcsmart.c: upsdrv_updateinfo(): if dstate_is_stale, try to reconnect every 60 retry attempts [#704] Signed-off-by: Jim Klimov --- drivers/apcsmart.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index d4392e10d6..8fa8ab4e75 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -2257,10 +2257,38 @@ void upsdrv_updateinfo(void) return; /* become aggressive after a few tries */ + if (!(last_worked % 60)) { + upslogx(LOG_WARNING, "Trying to reconnect to the UPS"); + dstate_setinfo("driver.state", "reconnect.trying"); + + upsdebugx(1, "%s: call upsdrv_cleanup", __func__); + /* dstate_setinfo("driver.state", "cleanup.upsdrv"); */ + upsdrv_cleanup(); + + upsdebugx(1, "%s: sleep 5 sec", __func__); + usleep(5000000); + + upsdebugx(1, "%s: call upsdrv_initups", __func__); + /* dstate_setinfo("driver.state", "init.device"); */ + upsdrv_initups(); + + upsdebugx(1, "%s: call upsdrv_initinfo", __func__); + /* dstate_setinfo("driver.state", "init.info"); */ + upsdrv_initinfo(); + + upsdebugx(1, "%s: call upsdrv_updateinfo", __func__); + dstate_setinfo("driver.state", "reconnect.updateinfo"); + /* dstate_setinfo("driver.state", "init.updateinfo"); */ + upsdrv_updateinfo(); + + dstate_setinfo("driver.state", "init.quiet"); + } + upsdebugx(1, "%s: nudging UPS with 'Y', iteration #%d ...", __func__, last_worked); - if (!smartmode(1)) + if (!smartmode(1)) { return; + } last_worked = 0; } From 9c2258b1ebfda29f85ceb76c5f1150df77d49953 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 03:21:59 +0200 Subject: [PATCH 07/15] drivers/apcsmart.c: upsdrv_updateinfo(): if dstate_is_stale, log if we fix it [#704] Signed-off-by: Jim Klimov --- drivers/apcsmart.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/apcsmart.c b/drivers/apcsmart.c index 8fa8ab4e75..b91cb6c195 100644 --- a/drivers/apcsmart.c +++ b/drivers/apcsmart.c @@ -2290,6 +2290,9 @@ void upsdrv_updateinfo(void) return; } + if (last_worked > 10) + upslogx(LOG_WARNING, "%s: recovered the connection", __func__); + upsdebugx(1, "%s: recovered the connection", __func__); last_worked = 0; } From 532fe02df7e1df19b21e7221474edda321e05b12 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Mon, 29 Jul 2024 03:34:28 +0200 Subject: [PATCH 08/15] NEWS.adoc: document the recent fixes for apcsmart [#704] Signed-off-by: Jim Klimov --- NEWS.adoc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.adoc b/NEWS.adoc index 9cbee8838b..9f10e4db86 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -101,6 +101,9 @@ https://github.com/networkupstools/nut/milestone/11 - apcsmart updates: * Revised code to use `strncpy()` and avoid potential overflows that are possible with `strcpy()` used before. [PR #2564] + * Lost communications led to a logging flood, should not anymore. + In fact, the driver should try fully reconnecting upon getting into + a prolonged data stale condition. [issue #704, PR #2564] - added Visench C1K (using serial port converter with USB ID `1a86:7523`) as known supported by `nutdrv_qx` (Megatec protocol) since at least From 642dfc5fb926f6edd43d1ecb545d508c1051f8e1 Mon Sep 17 00:00:00 2001 From: desertwitch <24509509+desertwitch@users.noreply.github.com> Date: Tue, 27 Aug 2024 22:51:09 +0200 Subject: [PATCH 09/15] drivers/main.c: do not overwrite set allow_killpower flag with defaults Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com> --- drivers/main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/main.c b/drivers/main.c index 46c33cf432..3e2251f604 100644 --- a/drivers/main.c +++ b/drivers/main.c @@ -2681,7 +2681,9 @@ int main(int argc, char **argv) upslogx(LOG_WARNING, "Running as foreground process, not saving a PID file"); } - dstate_setinfo("driver.flag.allow_killpower", "0"); + if (dstate_getinfo("driver.flag.allow_killpower") == NULL) + dstate_setinfo("driver.flag.allow_killpower", "0"); + dstate_setflags("driver.flag.allow_killpower", ST_FLAG_RW | ST_FLAG_NUMBER); dstate_addcmd("driver.killpower"); From 8a40e4f6af9b9a4b93b109becb06ae317e5ebcab Mon Sep 17 00:00:00 2001 From: desertwitch <24509509+desertwitch@users.noreply.github.com> Date: Wed, 28 Aug 2024 08:46:49 +0200 Subject: [PATCH 10/15] drivers/main.c: comment for setting driver.flag.allow_killpower default Signed-off-by: desertwitch <24509509+desertwitch@users.noreply.github.com> --- drivers/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/main.c b/drivers/main.c index 3e2251f604..5c4753a116 100644 --- a/drivers/main.c +++ b/drivers/main.c @@ -2681,6 +2681,7 @@ int main(int argc, char **argv) upslogx(LOG_WARNING, "Running as foreground process, not saving a PID file"); } + /* May already be set by parsed configuration flag, only set default if not: */ if (dstate_getinfo("driver.flag.allow_killpower") == NULL) dstate_setinfo("driver.flag.allow_killpower", "0"); From dddc52d5198563273612d424bc0088e157f3af51 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sun, 1 Sep 2024 13:35:03 +0200 Subject: [PATCH 11/15] tests/NIT/nit.sh: use TABCHAR in regex for portability Signed-off-by: Jim Klimov --- tests/NIT/nit.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/NIT/nit.sh b/tests/NIT/nit.sh index 558ce377ce..707b5c4da0 100755 --- a/tests/NIT/nit.sh +++ b/tests/NIT/nit.sh @@ -61,6 +61,8 @@ export NUT_QUIET_INIT_NDE_WARNING ARG_FG="-F" if [ x"${NUT_FOREGROUND_WITH_PID-}" = xtrue ] ; then ARG_FG="-FF" ; fi +TABCHAR="`printf '\t'`" + log_separator() { echo "" >&2 echo "================================" >&2 @@ -128,7 +130,7 @@ isBusy_NUT_PORT() { return 1 fi - (netstat -an || sockstat -l || ss -tn || ss -n) 2>/dev/null | grep -E "[:.]${NUT_PORT}(\t| |\$)" > /dev/null \ + (netstat -an || sockstat -l || ss -tn || ss -n) 2>/dev/null | grep -E "[:.]${NUT_PORT}(${TABCHAR}| |\$)" > /dev/null \ && log_debug "isBusy_NUT_PORT() found that NUT_PORT=${NUT_PORT} is busy per netstat, sockstat or ss" \ && return From 805a216d20e18fad1597928d4adf8f68e9ba950d Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sun, 1 Sep 2024 13:43:26 +0200 Subject: [PATCH 12/15] tests/NIT/nit.sh: check that /proc/net/tcp* are readable, not just existing Builds in Android with Termux lack the permissions to see open ports. Signed-off-by: Jim Klimov --- tests/NIT/nit.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/NIT/nit.sh b/tests/NIT/nit.sh index 707b5c4da0..7d4f67e239 100755 --- a/tests/NIT/nit.sh +++ b/tests/NIT/nit.sh @@ -110,7 +110,7 @@ isBusy_NUT_PORT() { [ -n "${NUT_PORT}" ] || return log_debug "isBusy_NUT_PORT() Trying to report if NUT_PORT=${NUT_PORT} is used" - if [ -e /proc/net/tcp ] || [ -e /proc/net/tcp6 ]; then + if [ -r /proc/net/tcp ] || [ -r /proc/net/tcp6 ]; then # Assume Linux - hex-encoded # IPv4: # sl local_address rem_address st tx_queue rx_queue tr tm->when retrnsmt uid timeout inode From 494ef017a6d27112889930b74fe759ee3b77badc Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sun, 1 Sep 2024 13:45:32 +0200 Subject: [PATCH 13/15] tests/NIT/nit.sh: check with BASH before bailing out with available tools that did not see hits Builds in Android with Termux lack the permissions to see open ports. Signed-off-by: Jim Klimov --- tests/NIT/nit.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/NIT/nit.sh b/tests/NIT/nit.sh index 7d4f67e239..e24bac9116 100755 --- a/tests/NIT/nit.sh +++ b/tests/NIT/nit.sh @@ -138,13 +138,6 @@ isBusy_NUT_PORT() { && log_debug "isBusy_NUT_PORT() found that NUT_PORT=${NUT_PORT} is busy per lsof" \ && return - # Not busy... or no tools to confirm? - if (command -v netstat || command -v sockstat || command -v ss || command -v lsof) 2>/dev/null >/dev/null ; then - # at least one tool is present, so not busy - log_debug "isBusy_NUT_PORT() found that NUT_PORT=${NUT_PORT} is not busy per netstat, sockstat, ss or lsof" - return 1 - fi - # If the current shell interpreter is bash, it can do a bit of networking: if [ -n "${BASH_VERSION-}" ]; then # NOTE: Probing host names we use in upsd.conf @@ -166,6 +159,13 @@ isBusy_NUT_PORT() { log_warn "isBusy_NUT_PORT() tried with BASH-specific query, and port does not seem busy (or something else errored out)" fi + # Not busy... or no tools to confirm? (or no perms, or no lsof plugin)? + if (command -v netstat || command -v sockstat || command -v ss || command -v lsof) 2>/dev/null >/dev/null ; then + # at least one tool is present, so not busy + log_debug "isBusy_NUT_PORT() found that NUT_PORT=${NUT_PORT} is not busy per netstat, sockstat, ss or lsof" + return 1 + fi + # Assume not busy to not preclude testing in 100% of the cases log_warn "isBusy_NUT_PORT() can not say definitively, tools for checking NUT_PORT=$NUT_PORT are not available" return 1 From 9482e303f2782cde9b0bd1600dd013bac1f3c602 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sun, 1 Sep 2024 13:57:28 +0200 Subject: [PATCH 14/15] tests/NIT/nit.sh: default to run with BASH where available (and system shell is not it) Signed-off-by: Jim Klimov --- tests/NIT/nit.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/NIT/nit.sh b/tests/NIT/nit.sh index e24bac9116..0d04dd54e3 100755 --- a/tests/NIT/nit.sh +++ b/tests/NIT/nit.sh @@ -36,6 +36,19 @@ # # License: GPLv2+ +if [ -z "${BASH_VERSION-}" ] \ +&& (command -v bash) \ +&& [ x"${DEBUG_NONBASH-}" != xtrue ] \ +; then + # FIXME: detect and pass -x/-v options? + echo "WARNING: Re-execing in BASH (export DEBUG_NONBASH=true to avoid)" >&2 + exec bash $0 "$@" +fi + +if [ -n "${BASH_VERSION-}" ]; then + eval `echo "set -o pipefail"` +fi + TZ=UTC LANG=C LC_ALL=C From 7cd962434bc5410d2baa236aab89ba8d9765f2b9 Mon Sep 17 00:00:00 2001 From: Jim Klimov Date: Sun, 1 Sep 2024 15:58:20 +0200 Subject: [PATCH 15/15] NEWS.adoc: document fix for allow_killpower flag [#2605] Signed-off-by: Jim Klimov --- NEWS.adoc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS.adoc b/NEWS.adoc index cd67bfce9c..d42c2375fa 100644 --- a/NEWS.adoc +++ b/NEWS.adoc @@ -62,6 +62,8 @@ https://github.com/networkupstools/nut/milestone/11 regression. [issue #1904, issue #2484] * Fallback `localtime_r()` and `gmtime_r()` for some platform builds where a `*_s()` variant was available was not handled correctly. [PR #2583] + * A recently introduced `allow_killpower` did not actually work as an + `ups.conf` flag (only as a protocol command). [issue #2605, PR #2606] - development iterations of NUT should now identify with not only the semantic version of a preceding release, but with git-derived information about the