On Mon, Nov 12, 2012 at 5:15 PM, Pravin B Shelar <pshe...@nicira.com> wrote:
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 08823e2..b86de0a 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> +static const char *
> +tun_flag_to_string(uint32_t flags)
> +{
> + switch (flags) {
> + case OVS_TNL_F_DONT_FRAGMENT:
> + return "dont_fragment";
> + case OVS_TNL_F_CSUM:
> + return "csum";
> + case OVS_TNL_F_KEY:
> + return "key_present";
>
> Since these can get printed a lot it might be useful to shorten them
for readability. I would probably go with "df", "csum", and "key" (which
are also more similar to the arguments that we use for configuring the
tunnel ports).
> +static void
> +format_tun_flags(struct ds *ds, uint32_t flags)
> +{
>
It seems like it should be possible to merge together formatting and
parsing functions for both the tunnel flags and the slow path reason by
passing in the values as an argument. I see a couple issues in both of
them so this might help.
> + uint32_t bad = 0;
> +
> + ds_put_format(ds, "flags=");
> +
> + if (!flags) {
> + ds_put_format(ds, "0");
>
Can you use 0x0 here to make it consistent with the case where we can't
decode the flags?
> @@ -926,25 +976,61 @@ parse_odp_key_attr(const char *s, const struct simap
> *port_names,
>
> {
> char tun_id_s[32];
> - unsigned long long int flags;
> int tos, ttl;
> struct ovs_key_ipv4_tunnel tun_key;
> - int n = -1;
> + int n0 = -1;
>
> if (sscanf(s, "ipv4_tunnel(tun_id=%31[x0123456789abcdefABCDEF],"
> - "flags=%lli,src="IP_SCAN_FMT",dst="IP_SCAN_FMT
> - ",tos=%i,ttl=%i)%n", tun_id_s, &flags,
> + "src="IP_SCAN_FMT",dst="IP_SCAN_FMT
> + ",tos=%i,ttl=%i,%n", tun_id_s,
> IP_SCAN_ARGS(&tun_key.ipv4_src),
> IP_SCAN_ARGS(&tun_key.ipv4_dst), &tos, &ttl,
> - &n) > 0 && n > 0) {
> + &n0) > 0 && n0 > 0) {
> + int n = -1;
> +
> tun_key.tun_id = htonll(strtoull(tun_id_s, NULL, 0));
> - tun_key.tun_flags = flags;
> tun_key.ipv4_tos = tos;
> tun_key.ipv4_ttl = ttl;
> memset(&tun_key.pad, 0, sizeof tun_key.pad);
> - nl_msg_put_unspec(key, OVS_KEY_ATTR_IPV4_TUNNEL, &tun_key,
> - sizeof tun_key);
> - return n;
> +
> + s += n0;
> + if (sscanf(s, "flags=%n", &n) == 0 && n > 0) {
>
"flags" should always be present so can't we just include it as part of the
previous sscanf call?
> +
> + tun_key.tun_flags = 0;
> +
> + while (s[n] != ')') {
> + uint32_t bit;
> +
> + for (bit = 1; bit; bit <<= 1) {
> + const char *flag = tun_flag_to_string(bit);
> + size_t len = strlen(flag);
> +
> + if (flag
> + && !strncmp(s + n, flag, len)
> + && (s[n + len] == ',' || s[n + len] == ')'))
> + {
> + tun_key.tun_flags |= bit;
> + n += len + (s[n + len] == ',');
> + break;
> + }
> + }
> +
> + if (!bit) {
> + if (s[n] == '0' && s[n + 1] == ')') {
> + n++;
> + break;
> + }
> + return -EINVAL;
> + }
>
This won't be able to read back any values in hex that couldn't be decoded
even though the information is present. The slow path function has the
same issue but I think both should be able to handle it.
> + }
> + if (s[n + 1] != ')') {
> + return -EINVAL;
> + }
I think this is looking at the closing parenthesis of set in the case of
actions, which isn't part of what we're trying to decode here. It works
because we don't actually advance past it but I don't think it will work in
the case of flow keys.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
