[ovs-dev] [PATCH 2/4] ovs-vswitchd: Add support for setting OpenFlow datapath descriptions

Ben Pfaff blp at nicira.com
Mon Feb 1 17:16:33 UTC 2010


On Fri, Jan 29, 2010 at 04:48:27PM -0800, Justin Pettit wrote:
>  
> +#define DEFAULT_MFR_DESC "Nicira Networks, Inc."
> +#define DEFAULT_HW_DESC "Reference Implementation"
> +#define DEFAULT_SW_DESC VERSION BUILDNR
> +#define DEFAULT_SERIAL_DESC "None"
> +#define DEFAULT_DP_DESC "None"

I don't think that we should claim to be the reference implementation.
Why not change that string to "Open vSwitch"?

>  static void
> +bridge_update_desc(struct bridge *br)
> +{
> +    bool changed = false;
> +    const char *desc;
> +
> +    desc = cfg_get_string(0, "bridge.%s.mfr-desc", br->name);
> +    if (desc != br->mfr_desc) {

I think that this will be true every time.  Did you mean to use
strcmp()?

> +        free(br->mfr_desc);
> +        if (desc) {
> +            br->mfr_desc = xstrdup(desc);
> +        } else {
> +            br->mfr_desc = xstrdup(DEFAULT_MFR_DESC);
> +        }
> +        changed = true;
> +    }

This cut-and-paste work is crying out for a helper function.  Or you
could even turn the strings into an array and use a for loop.

> +    if (changed) {
> +        ofproto_set_desc(br->ofproto, br->mfr_desc, br->hw_desc,
> +                br->sw_desc, br->serial_desc, br->dp_desc);
> +    }

Is there a good reason for the bridge to only call ofproto_set_desc() if
the values have changed?  If not, then the bridge doesn't have to keep
its own copies of those strings at all.

It might be easier to make ofproto_set_desc() itself use the default
values if NULLs are passed in.




More information about the dev mailing list