Merge 7755827 into c81b489

jimklimov · web-flow · commit 904fe55c555c · 2025-07-14T12:41:11.000+02:00
diff --git a/NEWS.adoc b/NEWS.adoc
@@ -175,12 +175,20 @@ https://github.com/networkupstools/nut/milestone/9
      [issue #2708]
 
  - `snmp-ups` driver updates:
+   * Added support for "fun"/"nuf" methods called from mapping tables to
+     report back to the driver that an argument value was not supported,
+     so `setvar()` or `instcmd()` can not proceed safely and should return
+     `STAT_SET_CONVERSION_FAILED` or `STAT_INSTCMD_CONVERSION_FAILED`. [#3017]
    * Fixed `ups.test.date` to be semi-static in `apc-mib` mapping, so it
      would be queried more than once per driver up-time. [issue #3011]
    * Fixed debug-logging around `SU_FLAG_STATIC` entries to clarify when
      they get skipped. [issue #3011]
 
  - `usbhid-ups` driver updates:
+   * Added support for "fun"/"nuf" methods called from mapping tables to
+     report back to the driver that an argument value was not supported,
+     so `setvar()` or `instcmd()` can not proceed safely and should return
+     `STAT_SET_CONVERSION_FAILED` or `STAT_INSTCMD_CONVERSION_FAILED`. [#3017]
    * The `cps-hid` subdriver's existing mechanism for fixing broken report
      descriptors was extended to cover a newly reported case of nominal UPS
      power being incorrectly reported due to an unrealistically low maximum
diff --git a/drivers/snmp-ups.c b/drivers/snmp-ups.c
@@ -177,7 +177,7 @@ static const char *mibname;
 static const char *mibvers;
 
 #define DRIVER_NAME	"Generic SNMP UPS driver"
-#define DRIVER_VERSION	"1.36"
+#define DRIVER_VERSION	"1.37"
 
 /* driver description structure */
 upsdrv_info_t	upsdrv_info = {
@@ -2123,7 +2123,9 @@ bool_t load_mib2nut(const char *mib)
 	return FALSE;
 }
 
-/* find the OID value matching that INFO_* value */
+/* find the OID value matching that INFO_* value
+ * Return -1 and errno==EINVAL for unsupported inputs.
+ */
 long su_find_valinfo(info_lkp_t *oid2info, const char* value)
 {
 	info_lkp_t *info_lkp;
@@ -2135,14 +2137,19 @@ long su_find_valinfo(info_lkp_t *oid2info, const char* value)
 			upsdebugx(1, "%s: found %s (value: %s)",
 					__func__, info_lkp->info_value, value);
 
+			errno = 0;
 			return info_lkp->oid_value;
 		}
 	}
+
 	upsdebugx(1, "%s: no matching INFO_* value for this OID value (%s)", __func__, value);
+	errno = EINVAL;
 	return -1;
 }
 
-/* String reformatting function */
+/* String reformatting function.
+ * Return NULL and errno==EINVAL for unsupported inputs.
+ */
 const char *su_find_strval(info_lkp_t *oid2info, void *value)
 {
 #if WITH_SNMP_LKP_FUN
@@ -2151,19 +2158,27 @@ const char *su_find_strval(info_lkp_t *oid2info, void *value)
 		const char	*retvalue;
 
 		upsdebugx(2, "%s: using generic lookup function (string reformatting)", __func__);
+		errno = 0;
 		retvalue = oid2info->fun_vp2s(value);
-		upsdebugx(2, "%s: got value '%s'", __func__, retvalue);
+		upsdebugx(2, "%s: got value '%s'%s",
+			__func__, retvalue,
+			(errno == EINVAL ? ", invalid input" : "")
+		);
 		return retvalue;
 	}
 	upsdebugx(1, "%s: no result value for this OID string value (%s)", __func__, (char*)value);
 #else
 	NUT_UNUSED_VARIABLE(oid2info);
 	upsdebugx(1, "%s: no mapping function for this OID string value (%s)", __func__, (char*)value);
 #endif /* WITH_SNMP_LKP_FUN */
+
+	errno = EINVAL;
 	return NULL;
 }
 
-/* find the INFO_* value matching that OID numeric (long) value */
+/* Find the INFO_* value matching that OID numeric (long) value.
+ * Return NULL and errno==EINVAL for unsupported inputs.
+ */
 const char *su_find_infoval(info_lkp_t *oid2info, void *raw_value)
 {
 	info_lkp_t *info_lkp;
@@ -2175,8 +2190,12 @@ const char *su_find_infoval(info_lkp_t *oid2info, void *raw_value)
 		const char	*retvalue;
 
 		upsdebugx(2, "%s: using generic lookup function", __func__);
+		errno = 0;
 		retvalue = oid2info->fun_vp2s(raw_value);
-		upsdebugx(2, "%s: got value '%s'", __func__, retvalue);
+		upsdebugx(2, "%s: got value '%s'%s",
+			__func__, retvalue,
+			(errno == EINVAL ? ", invalid input" : "")
+		);
 		return retvalue;
 	}
 #endif /* WITH_SNMP_LKP_FUN */
@@ -2189,10 +2208,13 @@ const char *su_find_infoval(info_lkp_t *oid2info, void *raw_value)
 			upsdebugx(1, "%s: found %s (value: %ld)",
 					__func__, info_lkp->info_value, value);
 
+			errno = 0;
 			return info_lkp->info_value;
 		}
 	}
+
 	upsdebugx(1, "%s: no matching INFO_* value for this OID value (%ld)", __func__, value);
+	errno = EINVAL;
 	return NULL;
 }
 
@@ -2838,6 +2860,7 @@ bool_t daisychain_init(void)
 		{
 #if WITH_SNMP_LKP_FUN
 			devices_count = -1;
+			errno = 0;
 			/* First test if we have a generic lookup function
 			 * FIXME: Check if the field type is a string?
 			 */
@@ -2849,11 +2872,14 @@ bool_t daisychain_init(void)
 				upsdebugx(2, "%s: using generic string-to-long lookup function", __func__);
 				if (TRUE == nut_snmp_get_str(su_info_p->OID, buf, sizeof(buf), su_info_p->oid2info)) {
 					devices_count = su_info_p->oid2info->nuf_s2l(buf);
-					upsdebugx(2, "%s: got value '%ld'", __func__, devices_count);
+					upsdebugx(2, "%s: got value '%ld'%s",
+						__func__, devices_count,
+						(errno == EINVAL ? ", invalid input" : "")
+					);
 				}
 			}
 
-			if (devices_count == -1) {
+			if (devices_count == -1 || errno == EINVAL) {
 #endif /* WITH_SNMP_LKP_FUN */
 
 				if (nut_snmp_get_int(su_info_p->OID, &devices_count) == TRUE) {
@@ -3659,10 +3685,13 @@ bool_t su_ups_get(snmp_info_t *su_info_p)
  * @varname: name of variable or command to set the OID from
  * @val: value for settings, NULL for commands
 
- * Returns
- *   STAT_SET_HANDLED if OK,
- *   STAT_SET_INVALID or STAT_SET_UNKNOWN if the command / setting is not supported
- *   STAT_SET_FAILED otherwise
+ * Returns respectively (for instcmd or setvar activity):
+ *   STAT_INSTCMD_HANDLED or STAT_SET_HANDLED if OK,
+ *   STAT_*_CONVERSION_FAILED if the value was invalid
+ *       (not a number when expected one,
+ *        not a key in mapping table when used)
+ *   STAT_*_INVALID or STAT_*_UNKNOWN if the command / setting is not supported
+ *   STAT_*_FAILED otherwise
  */
 static int su_setOID(int mode, const char *varname, const char *val)
 {
@@ -3755,7 +3784,7 @@ static int su_setOID(int mode, const char *varname, const char *val)
 		upsdebugx(2, "daisychain %s for device.0 are not yet supported!",
 			(mode==SU_MODE_INSTCMD)?"command":"setting");
 		free(tmp_varname);
-		return STAT_SET_INVALID;
+		return (mode==SU_MODE_INSTCMD ? STAT_INSTCMD_INVALID : STAT_SET_INVALID);
 	}
 
 	/* Check if it is outlet / outlet.group, or standard variable */
@@ -3822,7 +3851,7 @@ static int su_setOID(int mode, const char *varname, const char *val)
 			/* out of bound item number */
 			upsdebugx(2, "%s: item is out of bound (%i / %i)",
 				__func__, item_number, total_items);
-			return STAT_SET_INVALID;
+			return (mode==SU_MODE_INSTCMD ? STAT_INSTCMD_INVALID : STAT_SET_INVALID);
 		}
 		/* find back the item template */
 		item_varname = (char *)xmalloc(SU_INFOSIZE);
@@ -3891,8 +3920,9 @@ static int su_setOID(int mode, const char *varname, const char *val)
 					tmp_info_p->OID, "%i", item_number);
 			}
 		}
-		/* else, don't return STAT_SET_INVALID for mode==SU_MODE_SETVAR since we
-		 * can be setting a server side variable! */
+		/* else, don't return STAT_SET_INVALID for mode==SU_MODE_SETVAR
+		 * since we can be setting a server side variable - but
+		 * note this consideration does not apply to instcmd's! */
 		else {
 			if (mode==SU_MODE_INSTCMD) {
 				free_info(su_info_p);
@@ -3917,15 +3947,15 @@ static int su_setOID(int mode, const char *varname, const char *val)
 		if (tmp_varname != NULL)
 			free(tmp_varname);
 
-		return STAT_SET_UNKNOWN;
+		return (mode==SU_MODE_INSTCMD ? STAT_INSTCMD_UNKNOWN : STAT_SET_UNKNOWN);
 	}
 
 	/* set value into the device, using the provided one, or the default one otherwise */
 	if (mode==SU_MODE_INSTCMD) {
 		/* Sanity check: commands should either have a value or a default */
 		if ( (val == NULL) && (su_info_p->dfl == NULL) ) {
 			upsdebugx(1, "%s: cannot execute command '%s': a provided or default value is needed!", __func__, varname);
-			return STAT_SET_INVALID;
+			return STAT_INSTCMD_INVALID;
 		}
 		upslog_INSTCMD_POWERSTATE_CHECKED(varname, val);
 	}
@@ -3937,19 +3967,24 @@ static int su_setOID(int mode, const char *varname, const char *val)
 		if (mode==SU_MODE_INSTCMD) {
 			if ( !str_to_long(val ? val : su_info_p->dfl, &value, 10) ) {
 				upsdebugx(1, "%s: cannot execute command '%s': value is not a number!", __func__, varname);
-				return STAT_SET_INVALID;
+				return STAT_INSTCMD_CONVERSION_FAILED;
 			}
 		}
 		else {
 			/* non string data may imply a value lookup */
 			if (su_info_p->oid2info) {
 				value = su_find_valinfo(su_info_p->oid2info, val ? val : su_info_p->dfl);
+				if (value == -1 && errno == EINVAL) {
+					upsdebugx(1, "%s: cannot set '%s': value %s is not supported by lookup mapping!",
+						__func__, varname, NUT_STRARG(val ? val : su_info_p->dfl));
+					return STAT_SET_CONVERSION_FAILED;
+				}
 			}
 			else {
 				/* Convert value and apply multiplier */
 				if ( !str_to_long(val, &value, 10) ) {
 					upsdebugx(1, "%s: cannot set '%s': value is not a number!", __func__, varname);
-					return STAT_SET_INVALID;
+					return STAT_SET_CONVERSION_FAILED;
 				}
 				value = (long)((double)value / su_info_p->info_len);
 			}
@@ -3965,23 +4000,26 @@ static int su_setOID(int mode, const char *varname, const char *val)
 
 	/* Process result */
 	if (status == FALSE) {
-		if (mode==SU_MODE_INSTCMD)
+		if (mode==SU_MODE_INSTCMD) {
 			upsdebugx(1, "%s: cannot execute command '%s'", __func__, varname);
-		else
+			retval = STAT_INSTCMD_FAILED;
+		}
+		else {
 			upsdebugx(1, "%s: cannot set value %s on OID %s", __func__, val, su_info_p->OID);
-
-		retval = STAT_SET_FAILED;
+			retval = STAT_SET_FAILED;
+		}
 	}
 	else {
-		retval = STAT_SET_HANDLED;
-		if (mode==SU_MODE_INSTCMD)
+		if (mode==SU_MODE_INSTCMD) {
 			upsdebugx(1, "%s: successfully sent command %s", __func__, varname);
-		else {
+			retval = STAT_INSTCMD_HANDLED;
+		} else {
 			upsdebugx(1, "%s: successfully set %s to \"%s\"", __func__, varname, val);
 
 			/* update info array: call dstate_setinfo, since flags and aux are
 			 * already published, and this saves us some processing */
 			dstate_setinfo(varname, "%s", val);
+			retval = STAT_SET_HANDLED;
 		}
 	}
 
@@ -4003,12 +4041,7 @@ int su_setvar(const char *varname, const char *val)
 
 	ret = su_setOID(SU_MODE_SETVAR, varname, val);
 
-	if (ret == STAT_SET_FAILED)
-		upslog_SET_FAILED(varname, val);
-	else if (ret == STAT_SET_UNKNOWN)
-		upslog_SET_UNKNOWN(varname, val);
-	else if (ret == STAT_SET_INVALID)
-		upslog_SET_INVALID(varname, val);
+	upslog_SET_RESULT(ret, varname, extradata);
 
 	return ret;
 }
@@ -4047,12 +4080,7 @@ int su_instcmd(const char *cmdname, const char *extradata)
 
 	ret = su_setOID(SU_MODE_INSTCMD, cmdname, extradata);
 
-	if (ret == STAT_INSTCMD_FAILED)
-		upslog_INSTCMD_FAILED(cmdname, extradata);
-	else if (ret == STAT_INSTCMD_UNKNOWN)
-		upslog_INSTCMD_UNKNOWN(cmdname, extradata);
-	else if (ret == STAT_INSTCMD_INVALID)
-		upslog_INSTCMD_INVALID(cmdname, extradata);
+	upslog_INSTCMD_RESULT(ret, cmdname, extradata);
 
 	return ret;
 }
diff --git a/drivers/upshandler.h b/drivers/upshandler.h
@@ -210,6 +210,23 @@ struct ups_handler
 		__func__, NUT_STRARG(cmdname), NUT_STRARG(extra));	\
 	} while(0)
 
+#define upslog_INSTCMD_CONVERSION_FAILED(cmdname, extra)	do {	\
+	upslogx(LOG_INSTCMD_CONVERSION_FAILED, "%s: parameter value [%s] is invalid for command [%s]",	\
+		__func__, NUT_STRARG(extra), NUT_STRARG(cmdname));	\
+	} while(0)
+
+/* Note: log only faults; nothing for successful handling */
+#define upslog_INSTCMD_RESULT(ret, cmdname, extra)	do {		\
+	if (ret == STAT_INSTCMD_FAILED)					\
+		upslog_INSTCMD_FAILED(cmdname, extradata);		\
+	else if (ret == STAT_INSTCMD_UNKNOWN)				\
+		upslog_INSTCMD_UNKNOWN(cmdname, extradata);		\
+	else if (ret == STAT_INSTCMD_INVALID)				\
+		upslog_INSTCMD_INVALID(cmdname, extradata);		\
+	else if (ret == STAT_INSTCMD_CONVERSION_FAILED)			\
+		upslog_INSTCMD_CONVERSION_FAILED(cmdname, extradata);	\
+	} while(0)
+
 /* setvar(), setcmd() et al -- note they MUST have an argument: */
 #define upsdebug_SET_STARTING(varname, value)	do {		\
 	upsdebugx(1, "Starting %s::%s('%s', '%s')",		\
@@ -231,4 +248,21 @@ struct ups_handler
 		__func__, NUT_STRARG(varname), NUT_STRARG(value));	\
 	} while(0)
 
+#define upslog_SET_CONVERSION_FAILED(cmdname, extra)	do {	\
+	upslogx(LOG_SET_CONVERSION_FAILED, "%s: value [%s] is invalid for variable [%s]",	\
+		__func__, NUT_STRARG(extra), NUT_STRARG(cmdname));	\
+	} while(0)
+
+/* Note: log only faults; nothing for successful handling */
+#define upslog_SET_RESULT(ret, cmdname, extra)	do {			\
+	if (ret == STAT_SET_FAILED)					\
+		upslog_SET_FAILED(cmdname, extradata);			\
+	else if (ret == STAT_SET_UNKNOWN)				\
+		upslog_SET_UNKNOWN(cmdname, extradata);			\
+	else if (ret == STAT_SET_INVALID)				\
+		upslog_SET_INVALID(cmdname, extradata);			\
+	else if (ret == STAT_SET_CONVERSION_FAILED)			\
+		upslog_SET_CONVERSION_FAILED(cmdname, extradata);	\
+	} while(0)
+
 #endif /* NUT_UPSHANDLER_H */
diff --git a/drivers/usbhid-ups.c b/drivers/usbhid-ups.c
