Code review comment for lp:~citrix-openstack/nova/vmware-vsphere-support

[Notes above were cut-off, continues here...]

> 2193 + for elem in vswitches:
> 2194 + try:
> 2195 + for nic_elem in elem.pnic:
> 2196 + if str(nic_elem).split('-')[-1].find(vlan_interface) != -1:
> 2197 + return elem.name
> 2198 + except Exception:
> 2199 + pass

Same thing, a more specific exception handler would be easier to follow,
modify later if needed.

If you're want to skip errors (no matter what), it might be better to log
unexpectd errors, for example

    try:
        <a bunch of code which can raise>
    except IndexError:
        pass # this is okay
    except Exception, e:
        logging.error(_('Unexpected exception raised %s') % e)

> 1750 + "ds": ds,

One space ---> thattaway.

> 2213 + for pnic in physical_nics:
> 2214 + if vlan_interface == pnic.device:
> 2215 + return True
> 2216 + return False

This is fine code, nice and readable. Just as a point of discussion, the `any`
builtin could be used as well:

    return any((vlan_interface == pnic.device) for pnic in physical_nics)

Either way is fine, IMHO.

> 2318 +try:
> 2319 + READ_CHUNKSIZE = client.BaseClient.CHUNKSIZE
> 2320 +except:
> 2321 + READ_CHUNKSIZE = 65536

Better to qualify the exception handler, something like:

    try:
        READ_CHUNKSIZE = client.BaseClient.CHUNKSIZE
    except AttributeError:
        READ_CHUNKSIZE = 65536

> 2544 + # Use this when VMware fixes their faulty wsdl

Per HACKING, please prefix with TODO(sateesh) or FIXME(sateesh).

> 1364 +class VirtualDisk(DataObject):
> 1365 + """
> 1366 + Virtual Disk class. Does nothing special except setting
> 1367 + __class__.__name__ to 'VirtualDisk'. Refer place where __class__.__name__
> 1368 + is used in the code.
> 1369 + """
> 1370 +
> 1371 + def __init__(self):
> 1372 + DataObject.__init__(self)
> 1373 +
> 1374 +
> 1375 +class VirtualDiskFlatVer2BackingInfo(DataObject):
> 1376 + """VirtualDiskFlatVer2BackingInfo class."""
> 1377 +
> 1378 + def __init__(self):
> 1379 + DataObject.__init__(self)
> 1380 +
> 1381 +
> 1382 +class VirtualLsiLogicController(DataObject):
> 1383 + """VirtualLsiLogicController class."""
> 1384 +
> 1385 + def __init__(self):
> 1386 + DataObject.__init__(self)

Since the __init__ methods are just callng up, you can omit them:

    class VirtualLsiLogicController(DataObject):
        """VirtualLsiLogicController class."""
        pass

> 1351 +class DataObject(object):
> 1352 + """Data object base class."""
> 1353 +
> 1354 + def __init__(self):
> 1355 + pass
> 1356 +
> 1357 + def __getattr__(self, attr):
> 1358 + return object.__getattribute__(self, attr)
> 1359 +
> 1360 + def __setattr__(self, attr, value):
> 1361 + object.__setattr__(self, attr, value)

Since the __init__ is a no-op, you can just omit it; same for __setattr__.

__getattr__ is doing something a little strange, it's calling __getattribute__
instead of __getattr__. If this is desired, it definitely should have a
"NOTE(sateesh)" explaining why it's necessary. That said, it doesn't seem
necessary, so, like __init__ and __setattr__, I think it can be removed as
well:

    class DataObject(object):
        """Data object base class."""
        pass

> 1166 +class FaultCheckers:

You probably want to derive from `object` so this is a new-style class:

    class FaultCheckers(object):

> 1173 + @classmethod
> 1174 + def retrieveproperties_fault_checker(self, resp_obj):

classmethods should use `cls` rather than `self` for first arg. That said, the
`self` isn't used by the method, so it would make more sense as a @staticmethod.

Since this really just a function (as indicated by staticmethod), you could
consider just treating it like a function rather than attaching it to the
FaultChecker class. It's perfectly acceptable in Python to have free-floating
functions.

Of course, this would depend on your future plans for FaultChecker.

> 1136 + Exception.__init__(self)
> 1484 + ManagedObject.__init__(self, "HostSystem")
> 1551 + ManagedObject.__init__(self, "Datacenter")
> 1566 + ManagedObject.__init__(self, "Task")
... [more cases]

Consider using the super() syntax, if possible.

> 1280 + _db_content[table][obj.obj] = obj

This is a rather odd looking line. Just sayin' :)

> 1283 +def _get_objects(type):

`type` is a Python built-in, if possible, it would be better not to bind that
to a local variable.

Better:

    def _get_objects(type_):

Even Better:

    def _get_objects(obj_type):

> 1291 +class Prop(object):
> 1292 + """Property Object base class."""
> 1293 +
> 1294 + def __init__(self):
> 1295 + self.name = None
> 1296 + self.val = None
> 1297 +
> 1298 + def setVal(self, val):
> 1299 + self.val = val
> 1300 +
> 1301 + def setName(self, name):
> 1302 + self.name = name

The setVal and setName setters are unecessary. Python has a philosophy of
"We are all consenting adults here"[1], meaning you can access those attributes directly. So:

    my_prop = Prop()

Less Ideal:
    my_prop.setVal(1)

Better:

    my_prop.val = 1

Of course, if you ever have to attach additional functionality to setting the
value, you can use object-properties, like:

    val = property(get_val, set_val, del_val)

[1] http://mail.python.org/pipermail/tutor/2003-October/025932.html

> 1314 + object.__setattr__(self, 'propSet', [])

Per naming conventions, `propSet` might be better as `prop_set`.

> 1667 + def __init__(self):
> 1668 + pass

Not needed.

> 1849 + try:
> 1850 + obj_ref = obj.obj
> 1851 + # This means that we are doing a search for the managed
> 1852 + # dataobjects of the type in the inventory
> 1853 + if obj_ref == "RootFolder":
> 1854 + for mdo_ref in _db_content[type]:
> 1855 + mdo = _db_content[type][mdo_ref]
> 1856 + # Create a temp Managed object which has the same ref
> 1857 + # as the parent object and copies just the properties
> 1858 + # asked for. We need .obj along with the propSet of
> 1859 + # just the properties asked for
> 1860 + temp_mdo = ManagedObject(mdo.objName, mdo.obj)
> 1861 + for prop in properties:
> 1862 + temp_mdo.set(prop, mdo.get(prop))
> 1863 + lst_ret_objs.append(temp_mdo)
> 1864 + else:
> 1865 + if obj_ref in _db_content[type]:
> 1866 + mdo = _db_content[type][obj_ref]
> 1867 + temp_mdo = ManagedObject(mdo.objName, obj_ref)
> 1868 + for prop in properties:
> 1869 + temp_mdo.set(prop, mdo.get(prop))
> 1870 + lst_ret_objs.append(temp_mdo)
> 1871 + except Exception:
> 1872 + continue

This is a very broad exception handler. Can you distinguish between exceptions
that you expect and are willing to explicitly ignore and everything else?
Would prefer:

    try:
        <chunk of code>
    except KnownException:
        continue

And if you want to pass on all exceptions, adding:

    except Exception, e:
        LOG.exception(e)
        continue

> 2150 +class NetworkHelper:

New-style class:

    class NetworkHelper(object):

> 2151 +
> 2152 + @classmethod
> 2153 + def get_network_with_the_name(cls, session, network_name="vmnet0"):

All of the @classmethods for NetworkHelper can be staticmethods since `cls`
isn't used. This is more specific and makes it easier to refactor later since
you're explicitly saying that know dependency exists on the class. Example:

    @staticmethod
    def get_network_with_the_name(session, network_name="vmnet0"):