[ovs-dev] [shadow 2/2] Avoid shadowing local variable names.
Ben Pfaff
blp at nicira.com
Mon Sep 20 16:40:40 UTC 2010
Thanks, I pushed these two out.
On Fri, Sep 17, 2010 at 11:56:15PM -0700, Justin Pettit wrote:
> Looks good.
>
> --Justin
>
>
> On Sep 2, 2010, at 10:12 AM, Ben Pfaff wrote:
>
> > All of these changes avoid using the same name for two local variables
> > within a same function. None of them are actual bugs as far as I can tell,
> > but any of them could be confusing to the casual reader.
> >
> > The one in lib/ovsdb-idl.c is particularly brilliant: inner and outer
> > loops both using (different) variables named 'i'.
> >
> > Found with GCC -Wshadow.
> > ---
> > lib/dpif-netdev.c | 1 -
> > lib/dynamic-string.c | 2 --
> > lib/json.c | 1 -
> > lib/learning-switch.c | 6 +++---
> > lib/netdev-linux.c | 6 +++---
> > lib/netlink.c | 10 +++++-----
> > lib/ofp-parse.c | 6 +++---
> > lib/ovsdb-idl.c | 8 ++++----
> > lib/process.c | 2 +-
> > lib/stream-fd.c | 2 +-
> > lib/stream-ssl.c | 6 +++---
> > ofproto/ofproto.c | 2 --
> > ovsdb/execution.c | 2 --
> > tests/test-csum.c | 3 +--
> > tests/test-ovsdb.c | 6 ++++--
> > utilities/ovs-controller.c | 6 ++----
> > utilities/ovs-openflowd.c | 4 ----
> > utilities/ovs-vsctl.c | 24 +++++++++++-------------
> > vswitchd/bridge.c | 4 ----
> > 19 files changed, 41 insertions(+), 60 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 323f364..3975b5a 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1104,7 +1104,6 @@ dp_netdev_modify_vlan_tci(struct ofpbuf *packet, uint16_t tci, uint16_t mask)
> > veh->veth_tci |= htons(tci);
> > } else {
> > /* Insert new 802.1Q header. */
> > - struct eth_header *eh = packet->l2;
> > struct vlan_eth_header tmp;
> > memcpy(tmp.veth_dst, eh->eth_dst, ETH_ADDR_LEN);
> > memcpy(tmp.veth_src, eh->eth_src, ETH_ADDR_LEN);
> > diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> > index 5f8054a..3af7fc9 100644
> > --- a/lib/dynamic-string.c
> > +++ b/lib/dynamic-string.c
> > @@ -147,8 +147,6 @@ ds_put_format_valist(struct ds *ds, const char *format, va_list args_)
> > if (needed < available) {
> > ds->length += needed;
> > } else {
> > - size_t available;
> > -
> > ds_reserve(ds, ds->length + needed);
> >
> > va_copy(args, args_);
> > diff --git a/lib/json.c b/lib/json.c
> > index 3b70e6b..5887f67 100644
> > --- a/lib/json.c
> > +++ b/lib/json.c
> > @@ -705,7 +705,6 @@ json_lex_number(struct json_parser *p)
> > *
> > * We suppress negative zeros as a matter of policy. */
> > if (!significand) {
> > - struct json_token token;
> > token.type = T_INTEGER;
> > token.u.integer = 0;
> > json_parser_input(p, &token);
> > diff --git a/lib/learning-switch.c b/lib/learning-switch.c
> > index e189f1e..4e7645d 100644
> > --- a/lib/learning-switch.c
> > +++ b/lib/learning-switch.c
> > @@ -220,10 +220,10 @@ lswitch_process_packet(struct lswitch *sw, struct rconn *rconn,
> > }
> > }
> > if (VLOG_IS_DBG_ENABLED()) {
> > - char *p = ofp_to_string(msg->data, msg->size, 2);
> > + char *s = ofp_to_string(msg->data, msg->size, 2);
> > VLOG_DBG_RL(&rl, "%016llx: OpenFlow packet ignored: %s",
> > - sw->datapath_id, p);
> > - free(p);
> > + sw->datapath_id, s);
> > + free(s);
> > }
> > }
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index e6036bf..7227f5d 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -1779,12 +1779,12 @@ netdev_linux_get_in6(const struct netdev *netdev_, struct in6_addr *in6)
> > if (file != NULL) {
> > const char *name = netdev_get_name(netdev_);
> > while (fgets(line, sizeof line, file)) {
> > - struct in6_addr in6;
> > + struct in6_addr in6_tmp;
> > char ifname[16 + 1];
> > - if (parse_if_inet6_line(line, &in6, ifname)
> > + if (parse_if_inet6_line(line, &in6_tmp, ifname)
> > && !strcmp(name, ifname))
> > {
> > - netdev_dev->in6 = in6;
> > + netdev_dev->in6 = in6_tmp;
> > break;
> > }
> > }
> > diff --git a/lib/netlink.c b/lib/netlink.c
> > index 4e83747..66c27b1 100644
> > --- a/lib/netlink.c
> > +++ b/lib/netlink.c
> > @@ -1036,19 +1036,19 @@ nl_policy_parse(const struct ofpbuf *msg, size_t nla_offset,
> >
> > type = nla->nla_type;
> > if (type < n_attrs && policy[type].type != NL_A_NO_ATTR) {
> > - const struct nl_policy *p = &policy[type];
> > + const struct nl_policy *e = &policy[type];
> > size_t min_len, max_len;
> >
> > /* Validate length and content. */
> > - min_len = p->min_len ? p->min_len : attr_len_range[p->type][0];
> > - max_len = p->max_len ? p->max_len : attr_len_range[p->type][1];
> > + min_len = e->min_len ? e->min_len : attr_len_range[e->type][0];
> > + max_len = e->max_len ? e->max_len : attr_len_range[e->type][1];
> > if (len < min_len || len > max_len) {
> > VLOG_DBG_RL(&rl, "%zu: attr %"PRIu16" length %zu not in "
> > "allowed range %zu...%zu",
> > offset, type, len, min_len, max_len);
> > return false;
> > }
> > - if (p->type == NL_A_STRING) {
> > + if (e->type == NL_A_STRING) {
> > if (((char *) nla)[nla->nla_len - 1]) {
> > VLOG_DBG_RL(&rl, "%zu: attr %"PRIu16" lacks null at end",
> > offset, type);
> > @@ -1060,7 +1060,7 @@ nl_policy_parse(const struct ofpbuf *msg, size_t nla_offset,
> > return false;
> > }
> > }
> > - if (!p->optional && attrs[type] == NULL) {
> > + if (!e->optional && attrs[type] == NULL) {
> > assert(n_required > 0);
> > --n_required;
> > }
> > diff --git a/lib/ofp-parse.c b/lib/ofp-parse.c
> > index cc1419a..e649d17 100644
> > --- a/lib/ofp-parse.c
> > +++ b/lib/ofp-parse.c
> > @@ -267,12 +267,12 @@ str_to_action(char *str, struct ofpbuf *b)
> > put_output_action(b, str_to_u32(arg));
> > } else if (!strcasecmp(act, "enqueue")) {
> > char *sp = NULL;
> > - char *port = strtok_r(arg, ":q", &sp);
> > + char *port_s = strtok_r(arg, ":q", &sp);
> > char *queue = strtok_r(NULL, "", &sp);
> > - if (port == NULL || queue == NULL) {
> > + if (port_s == NULL || queue == NULL) {
> > ovs_fatal(0, "\"enqueue\" syntax is \"enqueue:PORT:QUEUE\"");
> > }
> > - put_enqueue_action(b, str_to_u32(port), str_to_u32(queue));
> > + put_enqueue_action(b, str_to_u32(port_s), str_to_u32(queue));
> > } else if (!strcasecmp(act, "drop")) {
> > /* A drop action in OpenFlow occurs by just not setting
> > * an action. */
> > diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> > index 2132f9f..43ff947 100644
> > --- a/lib/ovsdb-idl.c
> > +++ b/lib/ovsdb-idl.c
> > @@ -433,13 +433,13 @@ ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl)
> > const struct ovsdb_idl_table *table = &idl->tables[i];
> > const struct ovsdb_idl_table_class *tc = table->class;
> > struct json *monitor_request, *columns;
> > - size_t i;
> > + size_t j;
> >
> > monitor_request = json_object_create();
> > columns = json_array_create_empty();
> > - for (i = 0; i < tc->n_columns; i++) {
> > - const struct ovsdb_idl_column *column = &tc->columns[i];
> > - if (table->modes[i] != OVSDB_IDL_MODE_NONE) {
> > + for (j = 0; j < tc->n_columns; j++) {
> > + const struct ovsdb_idl_column *column = &tc->columns[j];
> > + if (table->modes[j] != OVSDB_IDL_MODE_NONE) {
> > json_array_add(columns, json_string_create(column->name));
> > }
> > }
> > diff --git a/lib/process.c b/lib/process.c
> > index a201a88..377c396 100644
> > --- a/lib/process.c
> > +++ b/lib/process.c
> > @@ -517,7 +517,7 @@ process_run_capture(char **argv, char **stdout_log, char **stderr_log,
> > block_sigchld(&oldsigs);
> > pid = fork();
> > if (pid < 0) {
> > - int error = errno;
> > + error = errno;
> >
> > unblock_sigchld(&oldsigs);
> > VLOG_WARN("fork failed: %s", strerror(error));
> > diff --git a/lib/stream-fd.c b/lib/stream-fd.c
> > index 9410009..ef4dc8d 100644
> > --- a/lib/stream-fd.c
> > +++ b/lib/stream-fd.c
> > @@ -214,7 +214,7 @@ pfd_accept(struct pstream *pstream, struct stream **new_streamp)
> >
> > new_fd = accept(ps->fd, (struct sockaddr *) &ss, &ss_len);
> > if (new_fd < 0) {
> > - int retval = errno;
> > + retval = errno;
> > if (retval != EAGAIN) {
> > VLOG_DBG_RL(&rl, "accept: %s", strerror(retval));
> > }
> > diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c
> > index 70b15f0..9c7533d 100644
> > --- a/lib/stream-ssl.c
> > +++ b/lib/stream-ssl.c
> > @@ -385,7 +385,7 @@ do_ca_cert_bootstrap(struct stream *stream)
> >
> > file = fdopen(fd, "w");
> > if (!file) {
> > - int error = errno;
> > + error = errno;
> > VLOG_ERR("could not bootstrap CA cert: fdopen failed: %s",
> > strerror(error));
> > unlink(ca_cert.file_name);
> > @@ -402,7 +402,7 @@ do_ca_cert_bootstrap(struct stream *stream)
> > }
> >
> > if (fclose(file)) {
> > - int error = errno;
> > + error = errno;
> > VLOG_ERR("could not bootstrap CA cert: writing %s failed: %s",
> > ca_cert.file_name, strerror(error));
> > unlink(ca_cert.file_name);
> > @@ -921,7 +921,7 @@ pssl_accept(struct pstream *pstream, struct stream **new_streamp)
> >
> > new_fd = accept(pssl->fd, &sin, &sin_len);
> > if (new_fd < 0) {
> > - int error = errno;
> > + error = errno;
> > if (error != EAGAIN) {
> > VLOG_DBG_RL(&rl, "accept: %s", strerror(error));
> > }
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 844083d..e571bd4 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -1071,7 +1071,6 @@ ofproto_run1(struct ofproto *p)
> >
> > for (i = 0; i < 50; i++) {
> > struct ofpbuf *buf;
> > - int error;
> >
> > error = dpif_recv(p->dpif, &buf);
> > if (error) {
> > @@ -1122,7 +1121,6 @@ ofproto_run1(struct ofproto *p)
> >
> > retval = pvconn_accept(ofservice->pvconn, OFP_VERSION, &vconn);
> > if (!retval) {
> > - struct ofconn *ofconn;
> > struct rconn *rconn;
> > char *name;
> >
> > diff --git a/ovsdb/execution.c b/ovsdb/execution.c
> > index 7ce9a3f..a96abfc 100644
> > --- a/ovsdb/execution.c
> > +++ b/ovsdb/execution.c
> > @@ -103,8 +103,6 @@ ovsdb_execute(struct ovsdb *db, const struct json *params,
> > || !params->u.array.n
> > || params->u.array.elems[0]->type != JSON_STRING
> > || strcmp(params->u.array.elems[0]->u.string, db->schema->name)) {
> > - struct ovsdb_error *error;
> > -
> > if (params->type != JSON_ARRAY) {
> > error = ovsdb_syntax_error(params, NULL, "array expected");
> > } else {
> > diff --git a/tests/test-csum.c b/tests/test-csum.c
> > index 8c85458..eebc880 100644
> > --- a/tests/test-csum.c
> > +++ b/tests/test-csum.c
> > @@ -1,5 +1,5 @@
> > /*
> > - * Copyright (c) 2009 Nicira Networks.
> > + * Copyright (c) 2009, 2010 Nicira Networks.
> > *
> > * Licensed under the Apache License, Version 2.0 (the "License");
> > * you may not use this file except in compliance with the License.
> > @@ -134,7 +134,6 @@ main(void)
> > const uint16_t *data16 = (const uint16_t *) tc->data;
> > const uint32_t *data32 = (const uint32_t *) tc->data;
> > uint32_t partial;
> > - size_t i;
> >
> > /* Test csum(). */
> > assert(ntohs(csum(tc->data, tc->size)) == tc->csum);
> > diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> > index 18784a5..04db654 100644
> > --- a/tests/test-ovsdb.c
> > +++ b/tests/test-ovsdb.c
> > @@ -1095,7 +1095,7 @@ do_query_distinct(int argc OVS_UNUSED, char *argv[])
> > size_t n_classes;
> > struct json *json;
> > int exit_code = 0;
> > - size_t i, j, k;
> > + size_t i;
> >
> > /* Parse table schema, create table. */
> > json = unbox_json(parse_json(argv[1]));
> > @@ -1161,6 +1161,7 @@ do_query_distinct(int argc OVS_UNUSED, char *argv[])
> > for (i = 0; i < json->u.array.n; i++) {
> > struct ovsdb_row_set results;
> > struct ovsdb_condition cnd;
> > + size_t j;
> >
> > check_ovsdb_error(ovsdb_condition_from_json(ts, json->u.array.elems[i],
> > NULL, &cnd));
> > @@ -1171,6 +1172,8 @@ do_query_distinct(int argc OVS_UNUSED, char *argv[])
> > ovsdb_row_set_init(&results);
> > ovsdb_query_distinct(table, &cnd, &columns, &results);
> > for (j = 0; j < results.n_rows; j++) {
> > + size_t k;
> > +
> > for (k = 0; k < n_rows; k++) {
> > if (uuid_equals(ovsdb_row_get_uuid(results.rows[j]),
> > &rows[k].uuid)) {
> > @@ -1833,7 +1836,6 @@ do_idl(int argc, char *argv[])
> > for (i = 2; i < argc; i++) {
> > char *arg = argv[i];
> > struct jsonrpc_msg *request, *reply;
> > - int error;
> >
> > if (*arg == '+') {
> > /* The previous transaction didn't change anything. */
> > diff --git a/utilities/ovs-controller.c b/utilities/ovs-controller.c
> > index b18959a..40e2a80 100644
> > --- a/utilities/ovs-controller.c
> > +++ b/utilities/ovs-controller.c
> > @@ -107,7 +107,6 @@ main(int argc, char *argv[])
> > for (i = optind; i < argc; i++) {
> > const char *name = argv[i];
> > struct vconn *vconn;
> > - int retval;
> >
> > retval = vconn_open(name, OFP_VERSION, &vconn);
> > if (!retval) {
> > @@ -146,12 +145,10 @@ main(int argc, char *argv[])
> >
> > while (n_switches > 0 || n_listeners > 0) {
> > int iteration;
> > - int i;
> >
> > /* Accept connections on listening vconns. */
> > for (i = 0; i < n_listeners && n_switches < MAX_SWITCHES; ) {
> > struct vconn *new_vconn;
> > - int retval;
> >
> > retval = pvconn_accept(listeners[i], OFP_VERSION, &new_vconn);
> > if (!retval || retval == EAGAIN) {
> > @@ -171,7 +168,8 @@ main(int argc, char *argv[])
> > bool progress = false;
> > for (i = 0; i < n_switches; ) {
> > struct switch_ *this = &switches[i];
> > - int retval = do_switching(this);
> > +
> > + retval = do_switching(this);
> > if (!retval || retval == EAGAIN) {
> > if (!retval) {
> > progress = true;
> > diff --git a/utilities/ovs-openflowd.c b/utilities/ovs-openflowd.c
> > index 8cb50e4..945b11d 100644
> > --- a/utilities/ovs-openflowd.c
> > +++ b/utilities/ovs-openflowd.c
> > @@ -458,8 +458,6 @@ parse_options(int argc, char *argv[], struct ofsettings *s)
> > s->n_controllers = controllers.n;
> > s->controllers = xmalloc(s->n_controllers * sizeof *s->controllers);
> > if (argc > 1) {
> > - size_t i;
> > -
> > for (i = 0; i < s->n_controllers; i++) {
> > s->controllers[i] = controller_opts;
> > s->controllers[i].target = controllers.names[i];
> > @@ -468,8 +466,6 @@ parse_options(int argc, char *argv[], struct ofsettings *s)
> >
> > /* Sanity check. */
> > if (controller_opts.band == OFPROTO_OUT_OF_BAND) {
> > - size_t i;
> > -
> > for (i = 0; i < s->n_controllers; i++) {
> > if (!strcmp(s->controllers[i].target, "discover")) {
> > ovs_fatal(0, "Cannot perform discovery with out-of-band "
> > diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
> > index 4d50194..41ccf28 100644
> > --- a/utilities/ovs-vsctl.c
> > +++ b/utilities/ovs-vsctl.c
> > @@ -1305,12 +1305,11 @@ add_port(struct vsctl_context *ctx,
> >
> > get_info(ctx->ovs, &info);
> > if (may_exist) {
> > - struct vsctl_port *port;
> > + struct vsctl_port *vsctl_port;
> >
> > - port = find_port(&info, port_name, false);
> > - if (port) {
> > + vsctl_port = find_port(&info, port_name, false);
> > + if (vsctl_port) {
> > struct svec want_names, have_names;
> > - size_t i;
> >
> > svec_init(&want_names);
> > for (i = 0; i < n_ifaces; i++) {
> > @@ -1319,15 +1318,16 @@ add_port(struct vsctl_context *ctx,
> > svec_sort(&want_names);
> >
> > svec_init(&have_names);
> > - for (i = 0; i < port->port_cfg->n_interfaces; i++) {
> > - svec_add(&have_names, port->port_cfg->interfaces[i]->name);
> > + for (i = 0; i < vsctl_port->port_cfg->n_interfaces; i++) {
> > + svec_add(&have_names,
> > + vsctl_port->port_cfg->interfaces[i]->name);
> > }
> > svec_sort(&have_names);
> >
> > - if (strcmp(port->bridge->name, br_name)) {
> > + if (strcmp(vsctl_port->bridge->name, br_name)) {
> > char *command = vsctl_context_to_string(ctx);
> > vsctl_fatal("\"%s\" but %s is actually attached to bridge %s",
> > - command, port_name, port->bridge->name);
> > + command, port_name, vsctl_port->bridge->name);
> > }
> >
> > if (!svec_equal(&want_names, &have_names)) {
> > @@ -2767,8 +2767,8 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
> >
> > ds_chomp(ds, '\n');
> > for (j = 0; j < ds->length; j++) {
> > - int c = ds->string[j];
> > - switch (c) {
> > + int ch = ds->string[j];
> > + switch (ch) {
> > case '\n':
> > fputs("\\n", stdout);
> > break;
> > @@ -2778,7 +2778,7 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
> > break;
> >
> > default:
> > - putchar(c);
> > + putchar(ch);
> > }
> > }
> > putchar('\n');
> > @@ -2796,8 +2796,6 @@ do_vsctl(const char *args, struct vsctl_command *commands, size_t n_commands,
> >
> > if (wait_for_reload && status != TXN_UNCHANGED) {
> > for (;;) {
> > - const struct ovsrec_open_vswitch *ovs;
> > -
> > ovsdb_idl_run(idl);
> > OVSREC_OPEN_VSWITCH_FOR_EACH (ovs, idl) {
> > if (ovs->cur_cfg >= next_cfg) {
> > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
> > index 598b001..3f5e3d4 100644
> > --- a/vswitchd/bridge.c
> > +++ b/vswitchd/bridge.c
> > @@ -632,7 +632,6 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
> > struct odp_port *dpif_ports;
> > size_t n_dpif_ports;
> > struct shash cur_ifaces, want_ifaces;
> > - struct shash_node *node;
> >
> > /* Get the set of interfaces currently in this datapath. */
> > dpif_port_list(br->dpif, &dpif_ports, &n_dpif_ports);
> > @@ -765,7 +764,6 @@ bridge_reconfigure(const struct ovsrec_open_vswitch *ovs_cfg)
> > struct ovsrec_controller **controllers;
> > struct ofproto_sflow_options oso;
> > size_t n_controllers;
> > - size_t i;
> >
> > memset(&oso, 0, sizeof oso);
> >
> > @@ -2822,7 +2820,6 @@ bond_rebalance_port(struct port *port)
> > * smallest hashes instead of the biggest ones. There is little
> > * reason behind this decision; we could use the opposite sort
> > * order to shift away big hashes ahead of small ones. */
> > - size_t i;
> > bool order_swapped;
> >
> > for (i = 0; i < from->n_hashes; i++) {
> > @@ -3407,7 +3404,6 @@ port_reconfigure(struct port *port, const struct ovsrec_port *cfg)
> > trunks = NULL;
> > if (vlan < 0 && cfg->n_trunks) {
> > size_t n_errors;
> > - size_t i;
> >
> > trunks = bitmap_allocate(4096);
> > n_errors = 0;
> > --
> > 1.7.1
> >
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
>
More information about the dev
mailing list