Merge pull request networkupstools#2058 from jimklimov/issue-1369

Enhance USB device and subdriver matching in `usbhid-ups` to be on par with `nutdrv_qx`

NEWS.adoc

@@ -152,6 +152,11 @@ as part of https://github.com/networkupstools/nut/issues/1410 solution.
    * extended default ranges for max battery voltage when guessing [#1279]
 
  - usbhid-ups updates:
+   * added support for `subdriver` configuration option, to select the
+     USB HID subdriver for the device manually where automatic match
+     does not suffice (e.g. new devices for which no `vendorid`/`productid`
+     pair was built into any driver, or for different-capability devices
+     with same interface chips, notably "phoenixtec/liebert" and "mge") [#1369]
    * cps-hid subdriver now applies same report descriptor fixing logic to
      devices with ProductID 0x0601 as done earlier for 0x0501, to get the
      correct output voltage data [#1497]

clients/Makefile.am

@@ -12,8 +12,9 @@ $(top_builddir)/common/libcommonclient.la \
 $(top_builddir)/common/libparseconf.la: dummy
 	@cd $(@D) && $(MAKE) $(AM_MAKEFLAGS) $(@F)
 
-# by default, link programs in this directory with libcommon.a
-LDADD = $(top_builddir)/common/libcommon.la libupsclient.la $(NETLIBS)
+# by default, link programs in this directory with
+# the more compact libcommonclient.a bundle
+LDADD = $(top_builddir)/common/libcommonclient.la libupsclient.la $(NETLIBS)
 if WITH_SSL
   LDADD += $(LIBSSL_LIBS)
 endif

common/Makefile.am

@@ -76,6 +76,14 @@ libcommonclient_la_LIBADD = libparseconf.la @LTLIBOBJS@ @NETLIBS@
 libcommon_la_CFLAGS = $(AM_CFLAGS)
 libcommonclient_la_CFLAGS = $(AM_CFLAGS)
 
+if HAVE_LIBREGEX
+  libcommon_la_CFLAGS += $(LIBREGEX_CFLAGS)
+  libcommon_la_LIBADD += $(LIBREGEX_LIBS)
+
+  libcommonclient_la_CFLAGS += $(LIBREGEX_CFLAGS)
+  libcommonclient_la_LIBADD += $(LIBREGEX_LIBS)
+endif
+
 # Did the user request, and build env support, tighter integration with
 # libsystemd methods such as sd_notify()?
 if WITH_LIBSYSTEMD

common/common.c

@@ -2126,3 +2126,109 @@ void set_close_on_exec(int fd) {
 # endif
 #endif
 }
+
+/**** REGEX helper methods ****/
+
+int strcmp_null(const char *s1, const char *s2)
+{
+	if (s1 == NULL && s2 == NULL) {
+		return 0;
+	}
+
+	if (s1 == NULL) {
+		return -1;
+	}
+
+	if (s2 == NULL) {
+		return 1;
+	}
+
+	return strcmp(s1, s2);
+}
+
+#if (defined HAVE_LIBREGEX && HAVE_LIBREGEX)
+int compile_regex(regex_t **compiled, const char *regex, const int cflags)
+{
+	int	r;
+	regex_t	*preg;
+
+	if (regex == NULL) {
+		*compiled = NULL;
+		return 0;
+	}
+
+	preg = malloc(sizeof(*preg));
+	if (!preg) {
+		return -1;
+	}
+
+	r = regcomp(preg, regex, cflags);
+	if (r) {
+		free(preg);
+		return -2;
+	}
+
+	*compiled = preg;
+
+	return 0;
+}
+
+int match_regex(const regex_t *preg, const char *str)
+{
+	int	r;
+	size_t	len = 0;
+	char	*string;
+	regmatch_t	match;
+
+	if (!preg) {
+		return 1;
+	}
+
+	if (!str) {
+		string = xstrdup("");
+	} else {
+		/* skip leading whitespace */
+		for (len = 0; len < strlen(str); len++) {
+
+			if (!strchr(" \t\n", str[len])) {
+				break;
+			}
+		}
+
+		string = xstrdup(str+len);
+
+		/* skip trailing whitespace */
+		for (len = strlen(string); len > 0; len--) {
+
+			if (!strchr(" \t\n", string[len-1])) {
+				break;
+			}
+		}
+
+		string[len] = '\0';
+	}
+
+	/* test the regular expression */
+	r = regexec(preg, string, 1, &match, 0);
+	free(string);
+	if (r) {
+		return 0;
+	}
+
+	/* check that the match is the entire string */
+	if ((match.rm_so != 0) || (match.rm_eo != (int)len)) {
+		return 0;
+	}
+
+	return 1;
+}
+
+int match_regex_hex(const regex_t *preg, const int n)
+{
+	char	buf[10];
+
+	snprintf(buf, sizeof(buf), "%04x", n);
+
+	return match_regex(preg, buf);
+}
+#endif	/* HAVE_LIBREGEX */

configure.ac

@@ -193,13 +193,17 @@ if test ! -d "${auglensdir}"; then
    fi
 fi
 
-LIBREGEX_LIBS=''
+dnl ### NUT_CHECK_LIBREGEX ### Detect below as part of libusb etc.
+dnl ### LIBREGEX_LIBS=''
 dnl Disable Hotplug, DevD and udev support on Windows
 dnl (useful when cross-compiling)
 case "${target_os}" in
     *mingw* )
-        dnl TODO: Actually detect it? See also nut_check_libusb.m4 for same.
-        LIBREGEX_LIBS='-lregex'
+        dnl TODO: Actually detect it? See also nut_check_libregex.m4 for same.
+        dnl Here we assumed from practice that it is available...
+        dnl ### if test x"${LIBREGEX_LIBS}" = x ; then
+        dnl ###     LIBREGEX_LIBS='-lregex'
+        dnl ### fi
         hotplugdir=''
         udevdir=''
         devddir=''
@@ -1522,6 +1526,7 @@ dnl These checks are performed unconditionally, even if the corresponding
 dnl --with-* options are not given. This is because we cannot predict
 dnl what will be in the --with-drivers argument.
 
+NUT_CHECK_LIBREGEX
 NUT_ARG_WITH([usb], [build and install USB drivers, optionally require build with specified version of libusb library and API: (auto|libusb-0.1|libusb-1.0)], [auto])
 nut_usb_lib=""
 NUT_CHECK_LIBUSB
@@ -4068,6 +4073,14 @@ dnl # -Wno-padded -- NSPR and NSS headers get to issue lots of that
 dnl # -Wno-c++98-compat-pedantic -Wno-c++98-compat -- our C++ code uses nullptr
 dnl #    as requested by newer linters, and C++98 does not. We require C++11
 dnl #    or newer anyway, and skip building C++ library and test otherwise.
+dnl # -Wno-fuse-ld-path -- not much in our control what recipes the autotools
+dnl #    on the build host generate... this tries to avoid failures due to:
+dnl #      clang-13: error: '-fuse-ld=' taking a path is deprecated.
+dnl #      Use '--ld-path=' instead [-Werror,-Wfuse-ld-path]
+dnl # -Wno-unsafe-buffer-usage -- clang-16 introduced a check too smart for
+dnl #    its own good. It detects use of pointer aritmetics as arrays are
+dnl #    walked, which is indeed potentially dangerous. And also is nearly
+dnl #    unavoidable in C (at least not without major rewrites of the world).
 dnl ### Special exclusion picks for clang-medium (same as hard, plus...):
 dnl # -Wno-float-conversion -Wno-double-promotion -Wno-implicit-float-conversion
 dnl #    -- reduce noise due to floating-point literals like "3.14" being a C
@@ -4098,12 +4111,12 @@ AS_CASE(["${nut_enable_warnings}"],
         CXXFLAGS="${CXXFLAGS} -Wall"
         ],
     [clang-hard], [
-        CFLAGS="${CFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -pedantic"
-        CXXFLAGS="${CXXFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -Wno-c++98-compat-pedantic -Wno-c++98-compat"
+        CFLAGS="${CFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -pedantic -Wno-fuse-ld-path -Wno-unsafe-buffer-usage"
+        CXXFLAGS="${CXXFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -Wno-c++98-compat-pedantic -Wno-c++98-compat -Wno-fuse-ld-path -Wno-unsafe-buffer-usage"
         ],
     [clang-medium], [
-        CFLAGS="${CFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -pedantic -Wno-float-conversion -Wno-double-promotion -Wno-implicit-float-conversion -Wno-conversion -Wno-incompatible-pointer-types-discards-qualifiers"
-        CXXFLAGS="${CXXFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -Wno-c++98-compat-pedantic -Wno-c++98-compat"
+        CFLAGS="${CFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -pedantic -Wno-fuse-ld-path -Wno-unsafe-buffer-usage -Wno-float-conversion -Wno-double-promotion -Wno-implicit-float-conversion -Wno-conversion -Wno-incompatible-pointer-types-discards-qualifiers"
+        CXXFLAGS="${CXXFLAGS} -ferror-limit=0 -Wno-system-headers -Wall -Wextra -Weverything -Wno-disabled-macro-expansion -Wno-unused-macros -Wno-reserved-id-macro -Wno-padded -Wno-documentation -Wno-cast-qual -Wno-c++98-compat-pedantic -Wno-c++98-compat -Wno-fuse-ld-path -Wno-unsafe-buffer-usage"
         ],
     [clang-minimal], [
         CFLAGS="${CFLAGS} -ferror-limit=0 -Wall -Wextra -Wno-documentation"

docs/man/nutdrv_qx.txt

@@ -64,6 +64,9 @@ If you set stayoff in linkman:ups.conf[5] when FSD arises the UPS will call a *s
 Skip autodetection of the protocol to use and only use the one specified.
 Supported values: 'bestups', 'hunnox', 'masterguard', 'mecer', 'megatec', 'megatec/old', 'mustek', 'q1', 'voltronic', 'voltronic-qs', 'voltronic-qs-hex' and 'zinto'.
 +
+Run the driver program with the `--help` option to see the exact list of
+`protocol` values it would currently recognize.
++
 Note that if you end up using the 'q1' protocol, you may want to give a try to the 'mecer', 'megatec' and 'zinto' ones setting the <<old-blazer-protocols-options,*novendor*/*norating* flags>> (only one, or both).
 
 *pollfreq =* 'num'::
@@ -348,7 +351,12 @@ include::nut_usb_addvars.txt[]
 *subdriver =* 'string'::
 Select a serial-over-USB subdriver to use.
 You have a choice between *cypress*, *fabula*, *fuji*, *hunnox*, *ippon*, *krauler*, *phoenix*, *phoenixtec*, *sgs*, *snr*,  *armac* and *ablerex*.
-When using this option, it is mandatory to also specify the *vendorid* and *productid*.
++
+Run the driver program with the `--help` option to see the exact list of
+`subdriver` values it would currently recognize.
++
+NOTE: When using this option, it is mandatory to also specify the *vendorid*
+and *productid* matching parameters.
 
 *langid_fix =* 'value'::
 Apply the language ID workaround to the *krauler* subdriver.

docs/man/usbhid-ups.txt

@@ -54,6 +54,24 @@ This driver also supports the following optional settings:
 
 include::nut_usb_addvars.txt[]
 
+*subdriver*='regex'::
+Select the USB HID subdriver for the device manually, where automatic match
+by device attributes alone does not suffice (e.g. new devices for which no
+`vendorid`/`productid` pair was built into any driver -- but common USB HID
+support is anticipated, or for different-capability devices with same
+interface chips, notably "phoenixtec/liebert" and "mge").
++
+Run the driver program with the `--help` option to see the exact list of
+`subdriver` values it would currently recognize.
++
+NOTE: this option first checks for exact matches to subdriver identification
+strings, such as `"TrippLite HID 0.85"` (which are prone to bit-rot), and if
+there was no exact match -- retries with a case-insensitive extended regular
+expression.
++
+NOTE: When using this option, it is mandatory to also specify the *vendorid*
+and *productid* matching parameters.
+
 *offdelay*='num'::
 Set the timer before the UPS is turned off after the kill power command is
 sent (via the *-k* switch).

drivers/Makefile.am

@@ -38,6 +38,9 @@ endif
 if WITH_MODBUS
   AM_CFLAGS += $(LIBMODBUS_CFLAGS)
 endif
+if HAVE_LIBREGEX
+  AM_CFLAGS += $(LIBREGEX_CFLAGS)
+endif
 
 NUTSW_DRIVERLIST = dummy-ups clone clone-outlet apcupsd-ups skel
 SERIAL_DRIVERLIST = al175 bcmxcp belkin belkinunv bestfcom	\

drivers/nutdrv_qx.c

@@ -39,6 +39,7 @@
  */
 
 #include "config.h"
+#include <ctype.h>
 #include "main.h"
 #include "attribute.h"
 #include "nut_float.h"
@@ -2771,20 +2772,45 @@ void	upsdrv_shutdown(void)
 
 void	upsdrv_help(void)
 {
-#ifdef QX_USB
-	#ifndef TESTING
+#ifndef TESTING
 	size_t i;
 
+# ifdef QX_USB
+	/* Subdrivers have special SOMETHING_command() handling and
+	 * are listed in usbsubdriver[] array (just above in this
+	 * source file).
+	 */
 	printf("\nAcceptable values for 'subdriver' via -x or ups.conf in this driver: ");
-
 	for (i = 0; usbsubdriver[i].name != NULL; i++) {
 		if (i>0)
 			printf(", ");
 		printf("%s", usbsubdriver[i].name);
 	}
 	printf("\n\n");
-	#endif
-#endif
+# endif	/* QX_USB*/
+
+	/* Protocols are the first token from "name" field in
+	 * subdriver_t instances in files like nutdrv_qx_mecer.c
+	 */
+	printf("\nAcceptable values for 'protocol' via -x or ups.conf in this driver: ");
+	for (i = 0; subdriver_list[i] != NULL; i++) {
+		char	subdrv_name[SMALLBUF], *p;
+
+		/* Get rid of subdriver version */
+		snprintf(subdrv_name, sizeof(subdrv_name), "%.*s",
+			(int)strcspn(subdriver_list[i]->name, " "),
+			subdriver_list[i]->name);
+
+		/* lowercase the (ASCII) string */
+		for (p = subdrv_name; *p; ++p)
+			*p = tolower(*p);
+
+		if (i>0)
+			printf(", ");
+		printf("%s", subdrv_name);
+	}
+	printf("\n\n");
+#endif	/* TESTING */
 
 	printf("Read The Fine Manual ('man 8 nutdrv_qx')\n");
 }