On 04/17/2015 05:55 PM, Justin Pettit wrote:
>
>> 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>
This description sounds great, thanks.
>> 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
Done.
>> 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
Done.
>> + 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
Done.
>> @@ -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.
Removed.
>> + 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"?
Done.
Thanks!
--
Russell Bryant
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
