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 <b...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
