Unnecessary return statement in ovs_lib

[Mohammad Banikazemi]

[openstack-dev] [neutron] unnecessary return statement in ovs_lib
Thu Jan 16 02:48:46 UTC 2014

Came across the following issue while looking at ovs_lib [1]:

The BaseOVS class has the add_bridge() method which after creating an OVS
bridge, returns an OVSBridge object. BaseOVS class is only used by
OVSBridge defined in the same file. OVSBridge has a create() method that
calls the add_bridge() nethod mentioned earlier but do not use the return
value. (See the methods add_bridge and create below.)

What seems odd is the return statement at the end of add_bridge() which is
not used anywhere and doesn't make much sense as far as I can see but I may
be missing something. The OVSBase is never directly used anywhere in
Neutron directory. Of course the return does not do any harm beyond
creating an unused object but it looks to me that it should be removed
unless there is a good reason (or a potential future use case) for it.

class BaseOVS(object):
        ...
    def add_bridge(self, bridge_name):
        self.run_vsctl(["--", "--may-exist", "add-br", bridge_name])
        return OVSBridge(bridge_name, self.root_helper)

class OVSBridge(BaseOVS):
        ...
    def create(self):
        self.add_bridge(self.br_name)

[1]
https://github.com/openstack/neutron/blob/master/neutron/agent/linux/ovs_lib.py

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/71142
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=c1eb61b98a37a10775affa845185388f05e3ceb4
Submitter: Jenkins
Branch: master

commit c1eb61b98a37a10775affa845185388f05e3ceb4
Author: Mohammad Banikazemi <email address hidden>
Date: Tue Feb 4 18:04:32 2014 -0500

    Removes an incorrect and unnecessary return

    The current return statement creates a new object that is
    not used anywhere and does not provide a functionality

    Change-Id: Id53f6fbc8cc6fb38419e5616a352279f1a9b917f
    Closes-Bug: #1276367

[Maru Newby (maru)]

We're going to revert this. It was used in the functional tests. This wasn't caught because the functional tests are in a kind of limbo pending their move to a separate job, but I would encourage people trying to 'better' the codebase to search the entire tree before assuming that removal is appropriate.