[ovs-dev] [PATCH 1/8] bridge: Publish error count in database's rstp_statistics.

Russell Bryant rbryant at redhat.com
Thu Feb 19 18:05:17 UTC 2015


On 02/19/2015 12:47 PM, Ben Pfaff wrote:
> On Thu, Feb 19, 2015 at 12:41:48PM -0500, Russell Bryant wrote:
>> On 02/19/2015 12:19 PM, Ben Pfaff wrote:
>>> On Thu, Feb 19, 2015 at 10:45:21AM -0500, Russell Bryant wrote:
>>>> On 02/19/2015 10:34 AM, Russell Bryant wrote:
>>>>> On 02/19/2015 03:12 AM, Ben Pfaff wrote:
>>>>>> The lower layers count errors but until now nothing actually reported them.
>>>>>>
>>>>>> Found by inspection.
>>>>>>
>>>>>> Signed-off-by: Ben Pfaff <blp at nicira.com>
>>>>>> ---
>>>>>>  vswitchd/bridge.c | 8 +++++---
>>>>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
>>>>>> index 297c0dd..a143be1 100644
>>>>>> --- a/vswitchd/bridge.c
>>>>>> +++ b/vswitchd/bridge.c
>>>>>> @@ -1,4 +1,4 @@
>>>>>> -/* Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014 Nicira, Inc.
>>>>>> +/* Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2015 Nicira, Inc.
>>>>>>   *
>>>>>>   * Licensed under the Apache License, Version 2.0 (the "License");
>>>>>>   * you may not use this file except in compliance with the License.
>>>>>> @@ -2489,8 +2489,8 @@ port_refresh_rstp_status(struct port *port)
>>>>>>      struct ofproto *ofproto = port->bridge->ofproto;
>>>>>>      struct iface *iface;
>>>>>>      struct ofproto_port_rstp_status status;
>>>>>> -    char *keys[3];
>>>>>> -    int64_t int_values[3];
>>>>>> +    char *keys[4];
>>>>>
>>>>> Based on the code below, it looks like it would be nice to make this
>>>>> "const char *keys[4];".
>>>>>
>>>>> If that gets changed, it's passed to automatically generated code where
>>>>> the parameter is not const.  It's the 2nd parameter here:
>>>>>
>>>>> ovsrec_port_set_statistics(const struct ovsrec_port *row, char
>>>>> **key_statistics, const int64_t *value_statistics, size_t n_statistics)
>>>>>
>>>>> The second parameter is explicitly excluded from being made const, but
>>>>> it's not clear why.  Is there some C detail I'm forgetting, or does this
>>>>> seem safe?
>>>>>
>>>>> The following change results in the 2nd parameter above being made const
>>>>> and seems to still build without any warnings:
>>>>
>>>> Nevermind ... of course there's a bunch of warnings.  I just didn't set
>>>> -Werror.
>>>>
>>>> It looks like the below change wouldn't be desired, but maybe adding
>>>> const to just "char **" would be OK.  Of course, it's not important
>>>> anyway ...
>>>
>>> "const" has really weird semantics in cases with double or multiple
>>> pointers in C (C++ is actually more sane here) so I'm in the habit of
>>> leaving off const in such cases.  Otherwise you end up with casts in
>>> places where it seems like you shouldn't need them.  That's why I tend
>>> not to use them.
>>>
>>
>> Fair enough.  FWIW, here's the impact to the code if it were added.
>> It's not bad and actually drops a cast.
> 
> You're right, this is an improvement.  May I have a Signed-off-by to
> commit this?  A Signed-off-by has the following form and meaning, as
> described in CONTRIBUTING.md.  (This is the same form and meaning as
> used in Linux kernel development.)

Sure, I'll submit it to the list as a proper patch in a moment.

-- 
Russell Bryant



More information about the dev mailing list