Fix usbhid-ups instant commands that could work without non-trivial args before NUT v2.8.3

NEWS.adoc

@@ -39,7 +39,15 @@ https://github.com/networkupstools/nut/milestone/9
  - (expected) CI automation for use of data points in drivers that conform
    to patterns defined in link:docs/nut-names.txt[]
 
- - (expected) Bug fixes for fallout possible due to "fightwarn" effort in 2.8.0+
+ - Bug fixes for fallout possible due to "fightwarn" effort in 2.8.0+:
+   * In `usbhid-ups` sources, introduced optional `HU_FLAG_PARAM_REQUIRED` for
+     `setvar()` or `instcmd()` handling (and a `HU_TYPE_CMD_PARAM_REQUIRED`
+     shortcut) for setting in the mapping table flags, to specify variables
+     or instant commands that require an argument (either from caller or a
+     non-`NULL` default in the run-time table after device data discovery);
+     if the flag is not set, a zero value is assumed. Incomplete code was a
+     regression of NUT v2.8.3 causing some instant commands to fail. [#2860,
+     #2955]
 
  - Fix fallout of development in NUT v2.8.0 and/or v2.8.1 and/or v2.8.2 and/or
    v2.8.3:

docs/nut.dict

@@ -1,4 +1,4 @@
-personal_ws-1.1 en 3494 utf-8
+personal_ws-1.1 en 3495 utf-8
 AAC
 AAS
 ABI
@@ -488,6 +488,7 @@ HOWTO
 HPE
 HPUX
 HREF
+HU
 HUPCL
 HUZKVEIkHnC
 HV

drivers/usbhid-ups.c

@@ -29,7 +29,7 @@
  */
 
 #define DRIVER_NAME	"Generic HID driver"
-#define DRIVER_VERSION	"0.63"
+#define DRIVER_VERSION	"0.64"
 
 #define HU_VAR_WAITBEFORERECONNECT "waitbeforereconnect"
 
@@ -900,7 +900,7 @@ int instcmd(const char *cmdname, const char *extradata)
 			return instcmd("load.off.delay", dstate_getinfo("ups.delay.shutdown"));
 		}
 
-		upsdebugx(2, "instcmd: info element unavailable %s\n", cmdname);
+		upsdebugx(2, "instcmd: info element unavailable %s", cmdname);
 		return STAT_INSTCMD_INVALID;
 	}
 
@@ -911,33 +911,49 @@ int instcmd(const char *cmdname, const char *extradata)
 
 	/* Check if the item is an instant command */
 	if (!(hidups_item->hidflags & HU_TYPE_CMD)) {
-		upsdebugx(2, "instcmd: %s is not an instant command\n", cmdname);
+		upsdebugx(2, "instcmd: %s is not an instant command", cmdname);
 		return STAT_INSTCMD_INVALID;
 	}
 
 	/* If extradata is empty, use the default value from the HID-to-NUT table */
 	val = extradata ? extradata : hidups_item->dfl;
-	if (!val) {
-		upsdebugx(2, "instcmd: %s requires an explicit parameter\n", cmdname);
+	if (!val && hidups_item->hidflags & HU_FLAG_PARAM_REQUIRED) {
+		upsdebugx(2, "instcmd: %s requires an explicit or default parameter", cmdname);
 		return STAT_INSTCMD_CONVERSION_FAILED;
 	}
 
 	/* Lookup the new value if needed */
 	if (hidups_item->hid2info != NULL) {
+		/* item->nuf() is expected to handle NULL if it must */
 		value = hu_find_valinfo(hidups_item->hid2info, val);
 	} else {
-		value = atol(val);
+		if (!val) {
+			/* If we end up with atol(NULL) below, it should return
+			 * 0 on error anyway (on platforms where it would not
+			 * crash instead due to the NULL), so we make it portably
+			 * explicit here.
+			 */
+			/* FIXME: Look up data points (maybe via override.* or
+			 * default.* settings) for delay/etc. when handling
+			 * commands like shutdown.* or load.* ?
+			 */
+			upsdebugx(4, "instcmd: %s got no explicit nor default parameter, "
+				"but does not require one: falling back to 0", cmdname);
+			value = 0;
+		} else {
+			value = atol(val);
+		}
 	}
 
 	/* Actual variable setting */
 	if (HIDSetDataValue(udev, hidups_item->hiddata, value) == 1) {
-		upsdebugx(3, "instcmd: SUCCEED\n");
+		upsdebugx(3, "instcmd: SUCCEED");
 		/* Set the status so that SEMI_STATIC vars are polled */
 		data_has_changed = TRUE;
 		return STAT_INSTCMD_HANDLED;
 	}
 
-	upsdebugx(3, "instcmd: FAILED\n"); /* TODO: HANDLED but FAILED, not UNKNOWN! */
+	upsdebugx(3, "instcmd: FAILED"); /* TODO: HANDLED but FAILED, not UNKNOWN! */
 	return STAT_INSTCMD_FAILED;
 }
 
@@ -953,45 +969,66 @@ int setvar(const char *varname, const char *val)
 	hidups_item = find_nut_info(varname);
 
 	if (hidups_item == NULL) {
-		upsdebugx(2, "setvar: info element unavailable %s\n", varname);
+		upsdebugx(2, "setvar: info element unavailable %s", varname);
 		return STAT_SET_UNKNOWN;
 	}
 
 	/* Checking item writability and HID Path */
 	if (!(hidups_item->info_flags & ST_FLAG_RW)) {
-		upsdebugx(2, "setvar: not writable %s\n", varname);
+		upsdebugx(2, "setvar: not writable %s", varname);
 		return STAT_SET_UNKNOWN;
 	}
 
 	/* handle server side variable */
 	if (hidups_item->hidflags & HU_FLAG_ABSENT) {
-		upsdebugx(2, "setvar: setting server side variable %s\n", varname);
+		upsdebugx(2, "setvar: setting server side variable %s", varname);
 		dstate_setinfo(hidups_item->info_type, "%s", val);
 		return STAT_SET_HANDLED;
 	}
 
 	/* HU_FLAG_ABSENT is the only case of HID Path == NULL */
 	if (hidups_item->hidpath == NULL) {
-		upsdebugx(2, "setvar: ID Path is NULL for %s\n", varname);
+		upsdebugx(2, "setvar: ID Path is NULL for %s", varname);
 		return STAT_SET_UNKNOWN;
 	}
 
+	/* FIXME: This code did not use "dfl"; should it start to?
+	 * If val is empty, use the default value from the HID-to-NUT table */
+	/* if (!val) val = hidups_item->dfl; */
+
+	if (!val && hidups_item->hidflags & HU_FLAG_PARAM_REQUIRED) {
+		upsdebugx(2, "setvar: %s requires an explicit or default parameter", varname);
+		return STAT_SET_CONVERSION_FAILED;
+	}
+
 	/* Lookup the new value if needed */
 	if (hidups_item->hid2info != NULL) {
+		/* item->nuf() is expected to handle NULL if it must */
 		value = hu_find_valinfo(hidups_item->hid2info, val);
 	} else {
-		value = atol(val);
+		if (!val) {
+			/* If we end up with atol(NULL) below, it should return
+			 * 0 on error anyway (on platforms where it would not
+			 * crash instead due to the NULL), so we make it portably
+			 * explicit here.
+			 */
+			upsdebugx(4, "setvar: %s got no explicit nor default parameter, "
+				"but does not require one: falling back to 0", varname);
+			value = 0;
+		} else {
+			value = atol(val);
+		}
 	}
 
 	/* Actual variable setting */
 	if (HIDSetDataValue(udev, hidups_item->hiddata, value) == 1) {
-		upsdebugx(5, "setvar: SUCCEED\n");
+		upsdebugx(5, "setvar: SUCCEED");
 		/* Set the status so that SEMI_STATIC vars are polled */
 		data_has_changed = TRUE;
 		return STAT_SET_HANDLED;
 	}
 
-	upsdebugx(3, "setvar: FAILED\n"); /* FIXME: HANDLED but FAILED, not UNKNOWN! */
+	upsdebugx(3, "setvar: FAILED"); /* FIXME: HANDLED but FAILED, not UNKNOWN! */
 	return STAT_SET_UNKNOWN;
 }
 
@@ -1253,7 +1290,7 @@ void upsdrv_updateinfo(void)
 		ups_infoval_set(item, value);
 	}
 #ifdef DEBUG
-	upsdebugx(1, "took %.3f seconds handling interrupt reports...\n",
+	upsdebugx(1, "took %.3f seconds handling interrupt reports...",
 		interval());
 #endif
 	/* clear status buffer before beginning */
@@ -1289,7 +1326,7 @@ void upsdrv_updateinfo(void)
 
 	dstate_dataok();
 #ifdef DEBUG
-	upsdebugx(1, "took %.3f seconds handling feature reports...\n",
+	upsdebugx(1, "took %.3f seconds handling feature reports...",
 		interval());
 #endif
 }

drivers/usbhid-ups.h

@@ -4,6 +4,7 @@
  *  2003-2009 Arnaud Quette <http://arnaud.quette.free.fr/contact.html>
  *  2005-2006 Peter Selinger <[email protected]>
  *  2007-2009 Arjen de Korte <[email protected]>
+ *  2017-2025 Jim Klimov <[email protected]>
  *
  * This program was sponsored by MGE UPS SYSTEMS, and now Eaton
  *
@@ -174,16 +175,16 @@ typedef enum {
 
 typedef struct {
 	const char	*info_type;		/* NUT variable name */
-	int	info_flags;		/* NUT flags (to set in addinfo) */
-	int	info_len;		/* if ST_FLAG_STRING: length of the string */
-					/* if HU_TYPE_CMD: command value */
+	int		info_flags;		/* NUT flags (to set in addinfo) */
+	int		info_len;		/* if ST_FLAG_STRING: length of the string */
+						/* if HU_TYPE_CMD: command value */
 	const char	*hidpath;		/* Full HID Object path (or NULL for server side vars) */
-	HIDData_t *hiddata;		/* Full HID Object data (for caching purpose, filled at runtime) */
+	HIDData_t	*hiddata;		/* Full HID Object data (for caching purpose, filled at runtime) */
 	const char	*dfl;			/* if HU_FLAG_ABSENT: default value ; format otherwise */
-	unsigned long hidflags;		/* driver's own flags */
-	info_lkp_t *hid2info;		/* lookup table between HID and NUT values */
-								/* if HU_FLAG_ENUM is set, hid2info is also used
-								 * as enumerated values (dstate_addenum()) */
+	unsigned long	hidflags;		/* driver's own flags */
+	info_lkp_t	*hid2info;		/* lookup table between HID and NUT values */
+						/* if HU_FLAG_ENUM is set, hid2info is also used
+						 * as enumerated values (dstate_addenum()) */
 
 /*	char *info_HID_format;	*//* FFE: HID format for complex values */
 /*	interpreter interpret;	*//* FFE: interpreter fct, NULL if not needed  */
@@ -192,15 +193,20 @@ typedef struct {
 
 /* TODO: rework flags */
 #define HU_FLAG_STATIC			2		/* retrieve info only once. */
-#define HU_FLAG_SEMI_STATIC		4		/* retrieve info smartly */
+#define HU_FLAG_SEMI_STATIC		4		/* retrieve info smartly. */
 #define HU_FLAG_ABSENT			8		/* data is absent in the device, */
-							/* use default value. */
-#define HU_FLAG_QUICK_POLL		16		/* Mandatory vars	*/
+							/* so we use default value. */
+#define HU_FLAG_QUICK_POLL		16		/* Mandatory vars. */
 #define HU_FLAG_STALE			32		/* data stale, don't try too often. */
-#define HU_FLAG_ENUM			128		/* enum values exist */
+/* see 64 below */
+#define HU_FLAG_ENUM			128		/* enum values exist. */
+#define HU_FLAG_PARAM_REQUIRED		256		/* require during setvar() or
+							 * instcmd() that a non-trivial
+							 * value parameter is passed. */
 
 /* hints for su_ups_set, applicable only to rw vars */
-#define HU_TYPE_CMD				64		/* instant command */
+#define HU_TYPE_CMD			64		/* instant command */
+#define HU_TYPE_CMD_PARAM_REQUIRED	(HU_TYPE_CMD | HU_FLAG_PARAM_REQUIRED)	/* Shortcut for setting in the mapping tables */
 
 #define HU_CMD_MASK		0x2000