[ovs-dev] [PATCH 3/4] Implement initial Python bindings for Open vSwitch database.
Reid Price
reid at nicira.com
Tue Aug 23 23:44:08 UTC 2011
A few notes inline, any lack of response indicates wholehearted agreement
On Tue, Aug 23, 2011 at 2:42 PM, Ben Pfaff <blp at nicira.com> wrote:
> I just sent out a patch series that addresses most of these. I'll
> just respond to the ones that I didn't do anything about, below.
>
> If you've lost the message that I'm replying to, it's archived here:
> http://openvswitch.org/pipermail/dev/2010-October/003927.html
>
> On Wed, Oct 06, 2010 at 03:10:12AM -0700, Reid Price wrote:
> > +def read_pidfile(pidfile):
> > + """Opens and reads a PID from 'pidfile'. Returns the nonnegative
> PID if
> > + successful, otherwise a negative errno value."""
> > + try:
> > + file = open(pidfile, "r")
> > + except IOError, e:
> > + logging.warning("%s: open: %s" % (pidfile,
> os.strerror(e.errno)))
> > + return -e.errno
> > +
> > + # Python fcntl doesn't directly support F_GETLK so we have to just
> try
> > + # to lock it. If we get a conflicting lock that's "success";
> otherwise
> > + # the file is not locked.
> > + try:
> > + fcntl.lockf(file, fcntl.LOCK_EX | fcntl.LOCK_NB)
> > + # File isn't locked if we get here, so treat that as an error.
> > + logging.warning("%s: pid file is not locked" % pidfile)
> > + try:
> > + # As a side effect, this drops the lock.
> > + file.close()
> > + except IOError:
> > + pass
> > + return -errno.ESRCH
> > One thing that might help streamline the 'it is an error if we get here'
> > portion is the 'else' portion of try/except. It only gets executed if
> > there is no exception.
>
> This code got rewritten by the following commits in the meantime. The
> newer code is probably easier to understand.
>
> aacea8b daemon: Avoid races on pidfile creation.
> 00c0858 daemon: Integrate checking for an existing pidfile into
> daemonize_start(
> af9a144 daemon: Tolerate EINTR in fork_and_wait_for_startup().
> 18e124a daemon: Avoid redundant code in already_running().
> 2159de8 daemon: Write "already running" message to log also.
> 998bb65 xenserver: monitor-external-ids should run with --monitor
> e4bd5e2 daemon: Fix behavior of read_pidfile() for our own pidfile.
>
> > +def parse_opt(option, arg):
> > + if option == '--detach':
> > + set_detach()
> > + elif option == '--no-chdir':
> > + set_no_chdir()
> > + elif option == '--pidfile':
> > + set_pidfile(None)
> > + elif option == '--pidfile-name':
> > + set_pidfile(arg)
> > + elif option == '--overwrite-pidfile':
> > + ignore_existing_pidfile()
> > + elif option == '--monitor':
> > + set_monitor()
> > + else:
> > + return False
> > + return True
> > It seems like all the methods in this file might benefit from being
> > encapsulated in a class, but I still have 4500 lines to go =)
> > Also, this class is never imported - how is it used?
>
> It's imported by debian/ovs-monitor-ipsec and
> xenserver/usr_share_openvswitch_scripts_ovs-xapi-syn and some tests.
>
> > + def __cmp__(self, other):
> > + if not isinstance(other, Atom) or self.type != other.type:
> > + return NotImplemented
> > Oh, I just learned something - thanks =)
>
> I knew something about Python that you didn't? Wow.
>
> > If this is performance-sensitive, you may want to change some of the
> imports
> > up top from 'import ovs.db.X' to 'import ovs.db.X as X'. I've heard each
> '.'
> > requires a dereference, and is not well-optimized. This is standard
> practice
> > if there isn't any name collision to worry about.
> > I'm not sure what the actual gain is in practice, if it ever becomes an
> > issue, could try replacing ovs.db.X.Y with ovs_db_X_Y, and make a bunch
> > of globals to measure the possible gain - but as far as I know there
> > hasn't been an problem.
>
> For the code that I've written most recently, I've been trying to
> following Google's Python style guide. The way I read it, I thought
> that it forbade using "as" on imports. It's kind of a pain spelling
> out the whole names all the time, so if I'm misreading it, or if this
> style guideline is widely ignored, let me know and I'll shorten
> everything.
>
Yeah, I think we're both coming from the same source
http://code.google.com/p/soc/wiki/PythonStyleGuide#Module_and_package_imports
In most of the code we've been writing, we tend to use statements like
import lib.foo.bar.baz as baz
google's example does this in the form
from lib.foo.bar import baz
where baz is a module (baz.py exists) in the package. Again, the
suggestion was mostly from a performance perspective. I don't
really begrudge the chains unless they make all my code linewrap,
though I think the 'import x.y.z .. as z' version of the import avoids
masking dependencies and still allows for readable code.
>
> > +
> > + def to_json(self):
> > + if len(self.values) == 1 and not self.type.is_map():
> > + key = self.values.keys()[0]
> > + return key.to_json()
> > + elif not self.type.is_map():
> > + return ["set", [k.to_json() for k in
> sorted(self.values.keys())]]
> > + else:
> > + return ["map", [[k.to_json(), v.to_json()]
> > + for k, v in sorted(self.values.items())]]
> > Is it ok that from_json(to_json()) will change a set of 1 to a non-set
> > singleton? I assume so, I know you used sets with length 0/1 for
> optional values.
>
> This is OK. A set of 1 and a non-set singleton have the same meaning.
>
> > Can use enumerate in these two places, and drop the items() in favor of
> > keys() [implied] for the first. I assume you want to keep keys for the
> > second to preserve ordering, even though it isn't used.
> > for i, key in enumerate(sorted(self.values)):
> > s += key.cInitAtom("%s->keys[%d]" % (var, i))
> > ...
> > for i, (key, value) in enumerate(sorted(self.values.items())):
> > s += key.cInitAtom("%s->values[%d]" % (var, i))
> > if it isn't too much of a mouthful.
>
> I didn't know about enumerate. It's nice, thanks!
>
> > + def __error(self):
> > + self.session.force_reconnect()
> > TODO(Reid): Check back to make sure that this doesn't have an obvious
> > infinite-loop of reconnecting (e.g.: if there were two clients receiving
> > eachother's messages unexpectedly)
>
> It did! Thanks for pointing out the bug.
>
> > +class AtomicType(object):
> > + def __init__(self, name, default):
> > + self.name = name
> > + self.default = default
> > +
> > + @staticmethod
> > + def from_string(s):
> > + if s != "void":
> > + for atomic_type in ATOMIC_TYPES:
> > + if s == atomic_type.name:
> > + return atomic_type
> > + raise error.Error("\"%s\" is not an atomic type" % s)
> > This has potential to return '"void" is not an atomic type', is that
> expected?
>
> That's OK. The string "void" isn't part of the OVSDB protocol
> specification at all. It's only used internally to indicate that a
> type is missing. So that error message would be fine.
>
> > +def returnUnchanged(x):
> > + return x
> > +
> >
> > It seems that you might be able to benefit from an additional type for
> > each class. It could split up all of the if/elif/.../else code into
> > subclasses, and let you avoid initializing values you don't care about
> > for that type.
> > Note: I made a half-hearted example, but then deleted it, the gain in
> > readability / usability was not very significant, though the structure
> > was nice.
>
> I didn't actually understand this suggestion, but it sounds like you
> decided it wasn't beneficial anyhow.
>
Yeah, it was academically cleaner, but overgeneralized for what is here.
>
> > + def to_json(self):
> > + if not self.has_constraints():
> > + return self.type.to_json()
> > It seems a little surprising that this returns a string when there are
> > no constraints, but an object otherwise.
> > Note: Checked back on this after reading rest, don't remember any
> > places where that behavior seemed obviously correct.
>
> This actually matches up with the OVSDB protocol specification. A
> very simple type can just be a string; otherwise it has to be a
> dictionary.
>
Ah, thanks for the clarification.
>
> > + def is_valid(self):
> > + return (self.key.type != VoidType and self.key.is_valid() and
> > + (self.value is None or
> > + (self.value.type != VoidType and
> self.value.is_valid())) and
> > + self.n_min <= 1 and
> > + self.n_min <= self.n_max and
> > + self.n_max >= 1)
> > I assume that the minimum can only be 0 or 1?
>
> That's right, the minimum can be 0 or 1, the maximum can be anything
> from 1 on up.
>
> > + def is_scalar(self):
> > + return self.n_min == 1 and self.n_max == 1 and not self.value
> > I don't know of a good example, but if you had a value of '', this might
> > (incorrectly?) identify a map that must have 1 item as a scalar.
>
> self.value is always a BaseType object or None, so it's OK.
>
> > + def toEnglish(self, escapeLiteral=returnUnchanged):
> > + keyName = self.key.toEnglish(escapeLiteral)
> > + if self.value:
> > + valueName = self.value.toEnglish(escapeLiteral)
> > +
> > + if self.is_scalar():
> > + return keyName
> > + elif self.is_optional():
> > + if self.value:
> > + return "optional %s-%s pair" % (keyName, valueName)
> > + else:
> > + return "optional %s" % keyName
> > + else:
> > + if self.n_max == sys.maxint:
> > + if self.n_min:
> > + quantity = "%d or more " % self.n_min
> > + else:
> > + quantity = ""
> > + elif self.n_min:
> > + quantity = "%d to %d " % (self.n_min, self.n_max)
> > In other places you've spoiled us with a case for "exactly %d", is there
> > a reason it is omitted here?
>
> Since n_min <= 1 and n_max >= 1, the only intersecting value is 1.
> That's mostly handled by is_scalar(). It misses the case of a map
> that must have exactly 1 element, but we don't have any maps like
> that.
>
> > + else:
> > + quantity = "up to %d " % self.n_max
> > +
> > + if self.value:
> > + return "map of %s%s-%s pairs" % (quantity, keyName,
> valueName)
> > + else:
> > + if keyName.endswith('s'):
> > + plural = keyName + "es"
> > + else:
> > + plural = keyName + "s"
> > + return "set of %s%s" % (quantity, plural)
> > We should add a check here ;)
> > assert keyName not in ['ox', 'fish', 'data'] # 'octopus' is ok
>
> Ha ha, yes. We don't have any columns with those names yet :-)
One can dream =)
>
> > +def _unlink_files():
> > + for file in _files:
> > + _unlink(file)
> > Do you care if _unlink returns non-zero?
>
> It's risky to try to do too much from a signal handler.
> > Could use python 2.5 fanciness and do
> > with open(name, "w") as stream:
> > to_stream(obj, stream, pretty, sort_keys)
> > this will take care of the try/finally for you. I don't use this much
> > myself, though I should. Probably worth noting that Xen was using
> > python 2.4, last release I checked.
>
> Yeah, that's why I didn't try this.
>
> > + def disable(self, now):
> > + """Disables this FSM. Until 'fsm' is enabled again, self.run()
> will
> > + always return 0."""
> > + if self.state != Reconnect.Void:
> > + self._transition(now, Reconnect.Void
> > Could use properties for a lot of these. Some could even just be direct
> > read/write (e.g.: max_tries), not sure how much of these functions are
> for
> > the documentation of variables though.
>
> This is probably a good idea, but I didn't do it (yet).
>
> > Could avoid shadowing the built-in type 'tuple' by mangling to _tuple,
> > or by using:
> > def open_block((error, stream)):
> > Though that might be confused at a glance with a 2-argument function
> > def open_block(error, stream):
>
> I didn't know that worked. Neat, thank you.
>
Should note that Peter pointed out to me that the preferred mangling
is a trailing underscore, i.e.: "tuple_" instead of "_tuple".
>
> > + def __del__(self):
> > + # Don't delete the file: we might have forked.
> > + self.socket.close()
> > There is enough overlap between these two classes to justify a shared
> > base class, which would simplify this one to little besides the 'accept'
> > function. A 'supported_types = ["punix"] or ["unix"]' class variable
> > would suffice for the is_valid_name and open functions, and the
> > close/__del__ functions are identical.
>
> You're probably right.
>
> I'll probably have to extend this code to handle more than just unix
> domain sockets. I think that point would be a good time to work on
> factoring this out.
>
> > +_argv0 = sys.argv[0]
> > +PROGRAM_NAME = _argv0[_argv0.rfind('/') + 1:]
> > Could also use:
> > PROGRAM_NAME = sys.argv[0].rpartition('/')[2]
>
> In the meantime this got changed to:
> PROGRAM_NAME = os.path.basename(sys.argv[0])
> which seems all-around better.
>
> Thanks a lot for the review!
>
Thanks for taking time to look through all these and responding, I
appreciate it
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20110823/7c799928/attachment-0003.html>
More information about the dev
mailing list