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

[Russell Bryant]

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.

diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index 6c8aa43..67e8a4e 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -21,7 +21,9 @@ def annotateSchema(schemaFile, annotationFile):
     sys.stdout.write('\n')

 def constify(cType, const):
-    if (const and cType.endswith('*') and not cType.endswith('**')):
+    if (const
+        and cType.endswith('*') and
+        (cType == 'char **' or not cType.endswith('**'))):
         return 'const %s' % cType
     else:
         return cType
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index 297c0dd..7d9cf25 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -2262,7 +2262,7 @@ iface_refresh_cfm_stats(struct iface *iface)
                 reasons[j++] = cfm_fault_reason_to_str(reason);
             }
         }
-        ovsrec_interface_set_cfm_fault_status(cfg, (char **) reasons, j);
+        ovsrec_interface_set_cfm_fault_status(cfg, reasons, j);

         ovsrec_interface_set_cfm_flap_count(cfg, &cfm_flap_count, 1);

@@ -2307,7 +2307,7 @@ iface_refresh_stats(struct iface *iface)
     enum { N_IFACE_STATS = IFACE_STATS };
 #undef IFACE_STAT
     int64_t values[N_IFACE_STATS];
-    char *keys[N_IFACE_STATS];
+    const char *keys[N_IFACE_STATS];
     int n;

     struct netdev_stats stats;
@@ -2419,7 +2419,7 @@ port_refresh_stp_stats(struct port *port)
     struct ofproto *ofproto = port->bridge->ofproto;
     struct iface *iface;
     struct ofproto_port_stp_stats stats;
-    char *keys[3];
+    const char *keys[3];
     int64_t int_values[3];

     if (port_is_synthetic(port)) {
@@ -2489,7 +2489,7 @@ 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];
+    const char *keys[3];
     int64_t int_values[3];
     struct smap smap;

@@ -4728,7 +4728,7 @@ mirror_refresh_stats(struct mirror *m)
 {
     struct ofproto *ofproto = m->bridge->ofproto;
     uint64_t tx_packets, tx_bytes;
-    char *keys[2];
+    const char *keys[2];
     int64_t values[2];
     size_t stat_cnt = 0;

-- 
Russell Bryant