>
> I see that this script in several places can print error messages. It
> should probably print these to stderr (by adding >&2). Maybe it
> should prefix them by "$0:", also (or calculate "basename $0" as a
> variable early on and use that).
I agree. I will add them in v2.
>
> As I started looking this over, I found myself wondering how to use
> it. I see that the instructions are in the second patch. I would
> consider squashing the patches together.
Okay. I will incorporate your suggestions to the second patch of this
series and squash it with this one.
>
> Some of the ovs-vsctl calls might benefit from ovs-vsctl's -f and -d
> options, although I didn't really look carefully.
It indeed does. I was not aware of a couple of them.
>
> I am a little concerned about a hard-coded timeout of 5 seconds. It
> is possible for a database operation to take longer, if the system is
> busy or the database is very large:
We will need some sort of timeout. Otherwise, a wrapper script like
this can simply hang if ovs-vswitchd is not running.
I bumped it up to 60 seconds (thought I am not happy with such a large
value either).
>> +ovs_vsctl () {
>> + ovs-vsctl --timeout=5 "$@"
>
>
> Should we specify a particular user, group, and permissions for this
> directory? (I do not know what the correct ones are.)
You mean with 'mkdir -p /var/run/netns'? I would imagine that
directory being created by other scripts to control Docker too. Since,
Docker instructions specify just 'mkdir -p /var/run/netns', I imagine
others would just do that. I am not sure whether it has pitfalls
keeping it like this.
> Also, I think
> that some newer systems generally use /run instead of /var/run. I
> don't know whether they have adapted "ip" to use /run:
man page of ip-netns only mentions /var/run/netns.
> It looks like delete_netns_link and create_netns_link are called in
> pairs, with delete_netns_link cleaning up at the end of execution. Is
> it a good idea to use the shell "trap" command so that
> delete_netns_link gets called even if the script gets killed?
I think it makes sense. I will add it.
>
> I don't think the () are necessary here:
Old habits die hard. I will get rid of them.
>> + if (ovs_vsctl --may-exist add-br "$BRIDGE"); then :; else
>> + echo "Failed to create bridge $BRIDGE"
>> + exit 1
>> + fi
>
> These are pretty specific substrings. They are fixed, for docker?
>> + PORTNAME="${CONTAINER:0:8}${INTERFACE:0:5}"
The $CONTAINER could have a 65 char uuid. And in Linux, a interface
name can have a max of 15 chars. Since I am creating the port, I need
to give it any unique name. I think the above method has pitfalls. So
in v2, I am using a fresh uuidgen to create a new port name and use 13
characters from it.
>
> I don't think the () are necessary here either:
Corrected it.
> I am not sure why there is a "while" loop at the end of the script. I
> think that only one iteration is possible.
It is not needed. Removed it.
Thanks for the review.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
