[ovs-dev] [PATCH] geneve-map-rename: rename geneve-map to tlv-map.

[Jesse Gross]

On Mon, Dec 14, 2015 at 8:52 AM, Mengke Liu <mengke.liu at intel.com> wrote:
> This patch renames the command name related with geneve-map to a more generic
> name as following:
> add-geneve-map -> add-tlv-map
> del-geneve-map -> del-tlv-map
> dump-geneve-map -> dump-tlv-map
>
> It also renames the Geneve_table to tlv_table.
>
> By doing this renaming, the NSH variable context header(the same TLV format as
> Geneve) or other protocol can reuse the field tun_metadata<N> in the future.
>
> Signed-off-by: Mengke Liu <mengke.liu at intel.com>
> Signed-off-by: Ricky Li <ricky.li at intel.com>

I have just a couple of comments:

 * The G in the various commands and error codes actually stands for
Geneve. i.e. NXGTMC_ADD means Nicira eXtension Geneve Table Mod
Command. Presumably these should be updated as well.

 * It looks like you did a global find and replace of "Geneve" with
"TLV", which is good for making sure that you got everything, however,
it sometimes results in comments that are not very helpful because TLV
is very generic whereas Geneve at least gives some context. In places
where there is descriptive text such as in nicira-ext.h or
ovs-ofctl.8.in, I think we need to have a little more description of
what TLV is referring to (perhaps giving Geneve as an example). In
other places, we should make sure that the comments still make sense
grammatically. So please go over the substitutions and make
improvements as necessary.

> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
> index e55e524..ffac397 100644
> --- a/ovn/controller/ofctrl.c
> +++ b/ovn/controller/ofctrl.c
> @@ -92,7 +92,7 @@ static struct rconn_packet_counter *tx_counter;
>   * installed in the switch. */
>  static struct hmap installed_flows;
>
> -/* MFF_* field ID for our Geneve option.  In S_GENEVE_TABLE_MOD_SENT, this is
> +/* MFF_* field ID for our TLV option.  In S_TLV_TABLE_MOD_SENT, this is

I think that the first substitution should remain Geneve here instead
of TLV since OVN actually is using Geneve specifically.