> On Apr 17, 2015, at 1:17 PM, Russell Bryant <rbry...@redhat.com> wrote:
Looks good, just some minor things. > <h1>Logical Port Commands</h1> > <dl> > - <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var></dt> > + <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var> > [<var>parent</var>] [<var>tag</var>]</dt> > <dd> > Creates on <var>lswitch</var> a new logical port named > - <var>lport</var>. > + <var>lport</var>. If this port is a child port (for a > + container running inside a VM), specify the parent port > + and tag for identifying this port's traffic. > </dd> In "ovs-vsctl add-br", the case of specifying the parent is treated as a new command. Instead of changing this definition, you could add another "lport-add" that handles the parent case. Also, I wonder if we'll have other cases of using children, such as nested hypervisors. What about a definition like the following: <dt><code>lport-add</code> <var>lswitch</var> <var>lport</var> <var>parent</var> <var>tag</var></dt> <dd> Creates on <var>lswitch</var> a logical port named <var>lport</var> that is a child of <var>parent</var> that is identied with <var>tag</var>. This is useful in cases such as virtualized container environments where Open vSwitch does not have a direct connection to the container's port and it must be shared with the virtual machine's port. </dd> > Logical port commands:\n\ > - lport-add LSWITCH LPORT add logical port LPORT on LSWITCH\n\ > + lport-add LSWITCH LPORT [PARENT] [TAG]\n\ > + add logical port LPORT on LSWITCH\n\ I think it might be clearer if you show it as two separate commands (there's some precedence with ovs-vsctl): lport-add LSWITCH LPORT add logical port LPORT on LSWITCH lport-add LSWITCH LPORT PARENT TAG add logical port LPORT on LSWITCH with PARENT on TAG > lport-del LPORT delete LPORT from its attached switch\n\ > lport-list LSWITCH print the names of all logical ports on LSWITCH\n\ > + lport-get-parent LPORT Get the parent port name if set\n\ To be consistent, I'd use lower-case for "get". What about phrasing like: lport-get-parent LPORT get parent of LPORT if set > + lport-get-tag LPORT Get the port's tag if set\n\ Same here. What about phrasing like: lport-get-tag LPORT Get the LPORT's tag if set > @@ -251,15 +255,37 @@ do_lport_add(struct ovs_cmdl_context *ctx) > struct nbctl_context *nb_ctx = ctx->pvt; > struct nbrec_logical_port *lport; > const struct nbrec_logical_switch *lswitch; > + int64_t tag; > + > + /* Validate all of the input */ I don't know that this comment adds much. > + if (ctx->argc != 3 && ctx->argc != 5) { > + /* If a parent_name is specififed, a tag must be specified as well. > */ > + VLOG_WARN("Invalid arguments to lport-add."); > + return; > + } > + > + if (ctx->argc == 5) { > + /* Validate tag. */ > + if (!ovs_scan(ctx->argv[4], "%"SCNd64, &tag) || tag < 0 || tag > > 4095) { > + VLOG_WARN("Invalid tag for logical port '%s'", ctx->argv[4]); The phrasing makes it sound like the returned value is the logical port's name, not the tag. What if you just drop "for logical port"? Thanks, --Justin _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev