On Thu, Mar 01, 2012 at 04:30:38PM -0800, Ethan Jackson wrote:
> Many of the currently implemented Python daemons, and likely many
> daemons to be implemented in the future, could benefit from unixctl
> support even if only to implement "exit" and "version" commands.
> This patch implements unixctl in Python.
>
> Signed-off-by: Ethan Jackson <[email protected]>
I noticed that there's some code that insists on "string" objects in
various places, which makes me a little nervous because "string" and
"unicode" objects in Python seem almost interchangeable but they are
not the same and a test for one will not accept the other. There's a
lot of "if type(json) in [str, unicode]" type code in the OVS python
directory for that reason.
> +class _UnixctlCommand(object):
> + def __init__(self, usage, min_args, max_args, callback, aux):
> + self.usage = usage
> + self.min_args = min_args
> + self.max_args = max_args
> + self.callback = callback
> + self.aux = aux
> +
> +
> +def _unixctl_help(conn, unused_argv, unused_aux):
> + assert isinstance(conn, UnixctlConnection)
> + reply = "The available commands are:\n"
> + command_names = sorted(commands.keys())
> + for name in command_names:
> + reply += " "
> + usage = commands[name].usage
> + if usage:
> + reply += "%-23s %s" % (name, usage)
> + else:
> + reply += name
> + reply += "\n"
> + conn.reply(reply)
I keep hearing that string concatenation in Python is really slow.
How about this (untested):
class _UnixctlCommand(object):
def __init__(self, usage, min_args, max_args, callback, aux):
self.usage = usage
self.min_args = min_args
self.max_args = max_args
self.callback = callback
self.aux = aux
def help(self):
if self.usage:
return "%-23 %s" % (self.name, self.usage)
else:
return self.name
def _unixctl_help(conn, unused_argv, unused_aux):
assert isinstance(conn, UnixctlConnection)
conn.reply("The available commands are:\n" +
''.join([" %s\n" % commands[name].help()
for name in sorted(commands.keys())]))
> +def socket_name_from_target(target):
> + assert isinstance(target, string)
> +
> + if target[0] == "/":
> + return 0, target
""[0] yields IndexError. Would target.startswith("/") be safer?
In UnixctlConnection._reply_impl(), I think that "if not
body.endswith('\n')" would be more straightforward:
> + if len(body) and body[-1] != '\n':
> + body += "\n"
I don't quite understand the code below, in
UnixctlConnection._process_command(). First we check that all of the
arguments are "string" objects, then we convert them to strings with
"str"?
> + for param in params:
> + if not isinstance(param, string):
> + error = '"%s" command has non-string argument' % method
> + break
> +
> + if error is None:
> + str_params = [str(p) for p in params]
> + command.callback(self, str_params, command.aux)
> +
> + if error:
> + self.reply_error(error)
> +
> +
[...]
Is it OK to remove elements from a list you're iterating? (I doubt
it.)
> + for conn in self._conns:
> + error = conn.run()
> + if error and error != errno.EAGAIN:
> + conn._close()
> + self._conns.remove(conn)
Otherwise this looks good to me. Thank you for doing this work!
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
