Re: rt_ifp and pf(4)
On Sat, Nov 21, 2015 at 12:37:58PM +0100, Martin Pieuchot wrote: > On 20/11/15(Fri) 18:05, Alexandr Nedvedicky wrote: > > Hello, > > > > I have just nit comments, feel free to ignore them. > > > > 1) would it be possible to use closing #endif guards as follows: > > #endif /* NCARP */ > > Done. > thanks a lot. I'm fine with your patch. OK regards sasha > > it's detail, given at some point in future we will probably have to use > > if_put()/if_get() for ifp's bound to kif's (right?). > > Well not necessarily. kif have the same lifetime as an ifp and are easy > to garbage collect so there's no need for the moment to use index for > them. Indexes are useful when two objects having a different lifetime > need to be linked at some point. By using indexes/id/cookie we do not > increase the lifetime of an object like with references but this has the > cost of a layer of indirection. and thanks for clarification here.
Re: rt_ifp and pf(4)
On 20/11/15(Fri) 18:05, Alexandr Nedvedicky wrote: > Hello, > > I have just nit comments, feel free to ignore them. > > 1) would it be possible to use closing #endif guards as follows: > #endif /* NCARP */ Done. > 2) another nit here: > > @@ -5607,6 +5616,8 @@ pf_route(struct mbuf **m, struct pf_rule > > done: > > if (r->rt != PF_DUPTO) > > *m = NULL; > > + if (!r->rt) > > + if_put(ifp); > > rtfree(rt); > > return; > > > > I would probably use test as follows: > if (rt != NULL) > if_put(ifp); > > it's detail, given at some point in future we will probably have to use > if_put()/if_get() for ifp's bound to kif's (right?). Well not necessarily. kif have the same lifetime as an ifp and are easy to garbage collect so there's no need for the moment to use index for them. Indexes are useful when two objects having a different lifetime need to be linked at some point. By using indexes/id/cookie we do not increase the lifetime of an object like with references but this has the cost of a layer of indirection.
Re: rt_ifp and pf(4)
Hello,
I have just nit comments, feel free to ignore them.
1) would it be possible to use closing #endif guards as follows:
#endif /* NCARP */
2) another nit here:
> @@ -5607,6 +5616,8 @@ pf_route(struct mbuf **m, struct pf_rule
> done:
> if (r->rt != PF_DUPTO)
> *m = NULL;
> + if (!r->rt)
> + if_put(ifp);
> rtfree(rt);
> return;
>
I would probably use test as follows:
if (rt != NULL)
if_put(ifp);
it's detail, given at some point in future we will probably have to use
if_put()/if_get() for ifp's bound to kif's (right?). My point here is we call
if_get() after we successfully obtain rt.
Apart those two everything is OK, since those two are nits, it's up to you if
you go for my suggestions.
regards
sasha
On Thu, Nov 19, 2015 at 12:07:38PM +0100, Martin Pieuchot wrote:
> Stop using rt_ifp. While here put some NCARP... ok?
>
> Index: net/pf.c
> ===
> RCS file: /cvs/src/sys/net/pf.c,v
> retrieving revision 1.950
> diff -u -p -r1.950 pf.c
> --- net/pf.c 12 Nov 2015 10:07:14 - 1.950
> +++ net/pf.c 19 Nov 2015 11:05:37 -
> @@ -36,6 +36,7 @@
> */
>
> #include "bpfilter.h"
> +#include "carp.h"
> #include "pflog.h"
> #include "pfsync.h"
> #include "pflow.h"
> @@ -2595,9 +2596,11 @@ pf_match_rcvif(struct mbuf *m, struct pf
> if (ifp == NULL)
> return (0);
>
> +#if NCARP > 0
> if (ifp->if_type == IFT_CARP && ifp->if_carpdev)
> kif = (struct pfi_kif *)ifp->if_carpdev->if_pf_kif;
> else
> +#endif
> kif = (struct pfi_kif *)ifp->if_pf_kif;
>
> if_put(ifp);
> @@ -5347,7 +5350,6 @@ pf_routable(struct pf_addr *addr, sa_fam
> struct sockaddr_in6 *dst6;
> #endif /* INET6 */
> struct rtentry *rt, *rt0 = NULL;
> - struct ifnet*ifp;
>
> check_mpath = 0;
> memset(, 0, sizeof(ss));
> @@ -5397,13 +5399,20 @@ pf_routable(struct pf_addr *addr, sa_fam
> ret = 0;
> rt = rt0;
> do {
> - if (rt->rt_ifp->if_type == IFT_CARP)
> - ifp = rt->rt_ifp->if_carpdev;
> - else
> - ifp = rt->rt_ifp;
> -
> - if (kif->pfik_ifp == ifp)
> + if (rt->rt_ifidx == kif->pfik_ifp->if_index) {
> ret = 1;
> +#if NCARP > 0
> + } else {
> + struct ifnet*ifp;
> +
> + ifp = if_get(rt->rt_ifidx);
> + if (ifp != NULL && ifp->if_type == IFT_CARP &&
> + ifp->if_carpdev == kif->pfik_ifp)
> + ret = 1;
> + if_put(ifp);
> +#endif
> + }
> +
> #ifndef SMALL_KERNEL
> rt = rtable_mpath_next(rt);
> #else
> @@ -5512,7 +5521,7 @@ pf_route(struct mbuf **m, struct pf_rule
> goto bad;
> }
>
> - ifp = rt->rt_ifp;
> + ifp = if_get(rt->rt_ifidx);
>
> if (rt->rt_flags & RTF_GATEWAY)
> dst = satosin(rt->rt_gateway);
> @@ -5607,6 +5616,8 @@ pf_route(struct mbuf **m, struct pf_rule
> done:
> if (r->rt != PF_DUPTO)
> *m = NULL;
> + if (!r->rt)
> + if_put(ifp);
> rtfree(rt);
> return;
>
> @@ -6312,9 +6323,11 @@ pf_test(sa_family_t af, int fwdir, struc
> if (!pf_status.running)
> return (PF_PASS);
>
> +#if NCARP > 0
> if (ifp->if_type == IFT_CARP && ifp->if_carpdev)
> kif = (struct pfi_kif *)ifp->if_carpdev->if_pf_kif;
> else
> +#endif
> kif = (struct pfi_kif *)ifp->if_pf_kif;
>
> if (kif == NULL) {
>
rt_ifp and pf(4)
Stop using rt_ifp. While here put some NCARP... ok?
Index: net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.950
diff -u -p -r1.950 pf.c
--- net/pf.c12 Nov 2015 10:07:14 - 1.950
+++ net/pf.c19 Nov 2015 11:05:37 -
@@ -36,6 +36,7 @@
*/
#include "bpfilter.h"
+#include "carp.h"
#include "pflog.h"
#include "pfsync.h"
#include "pflow.h"
@@ -2595,9 +2596,11 @@ pf_match_rcvif(struct mbuf *m, struct pf
if (ifp == NULL)
return (0);
+#if NCARP > 0
if (ifp->if_type == IFT_CARP && ifp->if_carpdev)
kif = (struct pfi_kif *)ifp->if_carpdev->if_pf_kif;
else
+#endif
kif = (struct pfi_kif *)ifp->if_pf_kif;
if_put(ifp);
@@ -5347,7 +5350,6 @@ pf_routable(struct pf_addr *addr, sa_fam
struct sockaddr_in6 *dst6;
#endif /* INET6 */
struct rtentry *rt, *rt0 = NULL;
- struct ifnet*ifp;
check_mpath = 0;
memset(, 0, sizeof(ss));
@@ -5397,13 +5399,20 @@ pf_routable(struct pf_addr *addr, sa_fam
ret = 0;
rt = rt0;
do {
- if (rt->rt_ifp->if_type == IFT_CARP)
- ifp = rt->rt_ifp->if_carpdev;
- else
- ifp = rt->rt_ifp;
-
- if (kif->pfik_ifp == ifp)
+ if (rt->rt_ifidx == kif->pfik_ifp->if_index) {
ret = 1;
+#if NCARP > 0
+ } else {
+ struct ifnet*ifp;
+
+ ifp = if_get(rt->rt_ifidx);
+ if (ifp != NULL && ifp->if_type == IFT_CARP &&
+ ifp->if_carpdev == kif->pfik_ifp)
+ ret = 1;
+ if_put(ifp);
+#endif
+ }
+
#ifndef SMALL_KERNEL
rt = rtable_mpath_next(rt);
#else
@@ -5512,7 +5521,7 @@ pf_route(struct mbuf **m, struct pf_rule
goto bad;
}
- ifp = rt->rt_ifp;
+ ifp = if_get(rt->rt_ifidx);
if (rt->rt_flags & RTF_GATEWAY)
dst = satosin(rt->rt_gateway);
@@ -5607,6 +5616,8 @@ pf_route(struct mbuf **m, struct pf_rule
done:
if (r->rt != PF_DUPTO)
*m = NULL;
+ if (!r->rt)
+ if_put(ifp);
rtfree(rt);
return;
@@ -6312,9 +6323,11 @@ pf_test(sa_family_t af, int fwdir, struc
if (!pf_status.running)
return (PF_PASS);
+#if NCARP > 0
if (ifp->if_type == IFT_CARP && ifp->if_carpdev)
kif = (struct pfi_kif *)ifp->if_carpdev->if_pf_kif;
else
+#endif
kif = (struct pfi_kif *)ifp->if_pf_kif;
if (kif == NULL) {
