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

Justin Pettit jpettit at nicira.com
Mon Feb 1 20:58:58 UTC 2010


On Feb 1, 2010, at 9:16 AM, Ben Pfaff wrote:

> 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"?

I was just copying what was there before, but your suggestion makes sense.  I've changed it.

>> 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()?

It won't be true if the user never specified the key.  I figure the common case is that these values are not overridden, so this would prevent allocating default strings and calling ofproto_set_desc() needlessly every time OVS is reloaded.  My first cut optimized things even more by detecting that the string hadn't changed, but then the code was getting more convoluted.  The likelihood is that this particular code will never really get any real use, since the config DB is going to make it all go away very soon.

>> +        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.

Yeah, agreed.

>> +    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.

That's probably true.  I'll rework it.

--Justin






More information about the dev mailing list