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