[ovs-dev] [PATCH 3/4] Implement initial Python bindings for Open vSwitch database.

[Ben Pfaff]

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.

> +
> +    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.

> +    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.

> +    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 :-)

> +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.

> +    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!