[ofa-general] Re: [PATCH v2] Add enum strings and *_str functions for enums

[Jack Morgenstein]

On Saturday 19 April 2008 00:54, Roland Dreier wrote:
> Thanks, I added a man page and changed things a little and committed the
> following:
> 
> commit 1c0b7ac0a6bbbe4d246ef4cf50ae31bde4929ba3
> Author: Ira Weiny <weiny2 at llnl.gov>
> Date:   Tue Apr 15 13:35:48 2008 -0700
> 
>     Add functions to convert enum values to strings
>     
>     Add ibv_xxx_str() functions to convert node type, port state, event
>     type and wc status enum values to strings.
>     
>     Signed-off-by: Ira K. Weiny <weiny2 at llnl.gov>
>     Signed-off-by: Roland Dreier <rolandd at cisco.com>
> 
>  
The change below (in the output format of the port state string)
is causing us problems (with all sorts of scripts based upon ibv_devinfo).
This only surfaced now because we're only just now bringing up the OFED 1.4 tree
based upon the current libibverbs and kernel 2.6.26.

Unfortunately, I missed this change when you posted it to the list in April.

I assume that the only reason for the change below, aside from adding the defer state, was cosmetic.
Is it possible to change the output strings so that for previously defined port states,
the output remains what it was previously?

(I don't see this as any different from other userspace library backwards-compatibility issues,
the change breaks existing scripts).

To avoid breaking the scripts, we would need as a minimum: 

+	static const char *const port_state_str[] = {
+		[IBV_PORT_NOP]		= "no state change (NOP)",
+		[IBV_PORT_DOWN]		= "PORT_DOWN",
+		[IBV_PORT_INIT]		= "PORT_INIT",
+		[IBV_PORT_ARMED]	= "PORT_ARMED",
+		[IBV_PORT_ACTIVE]	= "PORT_ACTIVE",
+		[IBV_PORT_ACTIVE_DEFER]	= "active defer"
+	};
+
+	if (port_state < IBV_PORT_NOP || port_state > IBV_PORT_ACTIVE_DEFER)
+		return "invalid state";

I have no preferences for "active defer", but you might consider
"PORT_ACTIVE_DEFER" for consistency. Our scripts do not work with the ACTIVE_DEFER
state. (100% backwards compatibility would dictate that the active_defer state and
the NOP state also return "invalid state" -- but this is clearly impossible to do).

Please note also that "unknown" was previously "invalid state".
- Jack

> -static const char *port_state_str(enum ibv_port_state pstate)
> -{
> -	switch (pstate) {
> -	case IBV_PORT_DOWN:   return "PORT_DOWN";
> -	case IBV_PORT_INIT:   return "PORT_INIT";
> -	case IBV_PORT_ARMED:  return "PORT_ARMED";
> -	case IBV_PORT_ACTIVE: return "PORT_ACTIVE";
> -	default:              return "invalid state";
> -	}
> -}
> -
> +
> +const char *ibv_port_state_str(enum ibv_port_state port_state)
> +{
> +	static const char *const port_state_str[] = {
> +		[IBV_PORT_NOP]		= "no state change (NOP)",
> +		[IBV_PORT_DOWN]		= "down",
> +		[IBV_PORT_INIT]		= "init",
> +		[IBV_PORT_ARMED]	= "armed",
> +		[IBV_PORT_ACTIVE]	= "active",
> +		[IBV_PORT_ACTIVE_DEFER]	= "active defer"
> +	};
> +
> +	if (port_state < IBV_PORT_NOP || port_state > IBV_PORT_ACTIVE_DEFER)
> +		return "unknown";
> +
> +	return port_state_str[port_state];
> +}