Re: svn commit: r225586 - in head/sys: modules/netgraph/ipfw netgraph

[Alexander V. Chernikov]

Gleb Smirnoff wrote:
>   Alexander, Andrey,
> 
>   see a couple of comments below please.
...
> A> +  if (m->m_len < sizeof(struct ip) &&
> A> +      (m = m_pullup(m, sizeof(struct ip))) == NULL)
> A> +          return (EINVAL);
> 
> In most cases we return ENOBUFS in case if m_pullup() failure. Lesser amount
> of code uses ENOMEM and EINVAL. I personally hate EINVAL, since it is usually
> used as one-for-all error, and thus is meaningless for user.
Understood. So can we use more descriptive ENOENT in code below?

        tag = m_tag_locate(m, MTAG_IPFW_RULE, 0, NULL);
        if (tag == NULL) {
                NG_FREE_M(m);
                return (EINVAL);        /* XXX: find smth better */
        };


> 
> A> +  ip = mtod(m, struct ip *);
> A> +
....
> A> +          default:
> A>                    return (EINVAL);
> 
> Shouldn't you free the mbuf before returning?
Ups.

Please see an attached patch






Index: sys/netgraph/ng_ipfw.c
===================================================================
--- sys/netgraph/ng_ipfw.c      (revision 226165)
+++ sys/netgraph/ng_ipfw.c      (working copy)
@@ -242,7 +242,7 @@ ng_ipfw_rcvdata(hook_p hook, item_p item)
 
        if (m->m_len < sizeof(struct ip) &&
            (m = m_pullup(m, sizeof(struct ip))) == NULL)
-               return (EINVAL);
+               return (ENOBUFS);
 
        ip = mtod(m, struct ip *);
 
@@ -252,18 +252,14 @@ ng_ipfw_rcvdata(hook_p hook, item_p item)
 #ifdef INET
                case IPVERSION:
                        ip_input(m);
-                       break;
+                       return (0);
 #endif
 #ifdef INET6
                case IPV6_VERSION >> 4:
                        ip6_input(m);
-                       break;
+                       return (0);
 #endif
-               default:
-                       NG_FREE_M(m);
-                       return (EINVAL);
                }
-               return (0);
        } else {
                switch (ip->ip_v) {
 #ifdef INET
@@ -277,10 +273,12 @@ ng_ipfw_rcvdata(hook_p hook, item_p item)
                        return (ip6_output(m, NULL, NULL, 0, NULL,
                            NULL, NULL));
 #endif
-               default:
-                       return (EINVAL);
                }
        }
+
+       /* unknown IP protocol version */
+       NG_FREE_M(m);
+       return (EPROTONOSUPPORT);
 }
 
 static int

signature.asc
Description: OpenPGP digital signature