* [PATCH net-next 2/3] net: ipv6: also allow token to be set when device not ready
From: Daniel Borkmann @ 2013-04-09 13:47 UTC (permalink / raw)
To: davem; +Cc: netdev, hannes
In-Reply-To: <1365515236-7154-1-git-send-email-dborkman@redhat.com>
When we set the iftoken in inet6_set_iftoken(), we return -EINVAL
when the device does not have flag IF_READY. This is however not
necessary and rather an artificial usability barrier, since we
simply can set the token despite that, and in case the device is
ready, we just send out our rs, otherwise ifup et al. will do
this for us anyway.
Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
net/ipv6/addrconf.c | 22 ++++++++++++++++------
1 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 645bf31..713ebe3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4296,9 +4296,9 @@ static int inet6_fill_link_af(struct sk_buff *skb, const struct net_device *dev)
static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
{
- struct in6_addr ll_addr;
struct inet6_ifaddr *ifp;
struct net_device *dev = idev->dev;
+ bool update_rs = false;
if (token == NULL)
return -EINVAL;
@@ -4306,8 +4306,6 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
return -EINVAL;
if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
return -EINVAL;
- if (idev->dead || !(idev->if_flags & IF_READY))
- return -EINVAL;
if (!ipv6_accept_ra(idev))
return -EINVAL;
if (idev->cnf.rtr_solicits <= 0)
@@ -4320,11 +4318,23 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
write_unlock_bh(&idev->lock);
- ipv6_get_lladdr(dev, &ll_addr, IFA_F_TENTATIVE | IFA_F_OPTIMISTIC);
- ndisc_send_rs(dev, &ll_addr, &in6addr_linklocal_allrouters);
+ if (!idev->dead && (idev->if_flags & IF_READY)) {
+ struct in6_addr ll_addr;
+
+ ipv6_get_lladdr(dev, &ll_addr, IFA_F_TENTATIVE |
+ IFA_F_OPTIMISTIC);
+
+ /* If we're not ready, then normal ifup will take care
+ * of this. Otherwise, we need to request our rs here.
+ */
+ ndisc_send_rs(dev, &ll_addr, &in6addr_linklocal_allrouters);
+ update_rs = true;
+ }
write_lock_bh(&idev->lock);
- idev->if_flags |= IF_RS_SENT;
+
+ if (update_rs)
+ idev->if_flags |= IF_RS_SENT;
/* Well, that's kinda nasty ... */
list_for_each_entry(ifp, &idev->addr_list, if_list) {
--
1.7.1
^ permalink raw reply related
* [PATCH net-next 1/3] net: ipv6: minor: use in6addr_any in token init
From: Daniel Borkmann @ 2013-04-09 13:47 UTC (permalink / raw)
To: davem; +Cc: netdev, hannes
In-Reply-To: <1365515236-7154-1-git-send-email-dborkman@redhat.com>
Since we check for !ipv6_addr_any(&in6_dev->token) in
addrconf_prefix_rcv(), make the token initialization on
device setup more intuitive by using in6addr_any as an
initializer.
Suggested-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
net/ipv6/addrconf.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 65d8139..645bf31 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -422,7 +422,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
ipv6_regen_rndid((unsigned long) ndev);
}
#endif
- memset(ndev->token.s6_addr, 0, sizeof(ndev->token.s6_addr));
+ ndev->token = in6addr_any;
if (netif_running(dev) && addrconf_qdisc_ok(dev))
ndev->if_flags |= IF_READY;
--
1.7.1
^ permalink raw reply related
* [PATCH net-next 0/3] follow-up improvements for ipv6 tokens
From: Daniel Borkmann @ 2013-04-09 13:47 UTC (permalink / raw)
To: davem; +Cc: netdev, hannes
This set implements follow-up suggestions from Hannes for commit
f53adae4eae5ad9 (``net: ipv6: add tokenized interface identifier
support''). Tested by myself.
Daniel Borkmann (3):
net: ipv6: minor: use in6addr_any in token init
net: ipv6: also allow token to be set when device not ready
net: ipv6: only invalidate previously tokenized addresses
include/net/if_inet6.h | 2 ++
net/ipv6/addrconf.c | 31 ++++++++++++++++++++++---------
2 files changed, 24 insertions(+), 9 deletions(-)
^ permalink raw reply
* Re: be2net: GRO for non-inet protocols
From: Eric Dumazet @ 2013-04-09 13:44 UTC (permalink / raw)
To: Erik Hugne; +Cc: sathya.perla, subbu.seetharaman, ajit.khaparde, netdev
In-Reply-To: <20130409075532.GA20363@eerihug-hybrid.ki.sw.ericsson.se>
On Tue, 2013-04-09 at 09:55 +0200, Erik Hugne wrote:
> I haven't found any issues with the GRO support itself. It's only on
> the system with Emulex NIC's that i have problems.
> Unfortunately, I'm stuck to kernel 3.0.61 on these machines, newer kernels
> won't boot due to an unresolved driver issue with the hw raid controller :/
I suggested you try GRE tunnels on emulex NIC, to make sure the problem
is not coming from your changes. Nevermind ...
^ permalink raw reply
* Re: [PATCH 1/3 net-next RFC] selftest: add abstractions for net selftests
From: Sergei Shtylyov @ 2013-04-09 13:39 UTC (permalink / raw)
To: Alexandru Copot; +Cc: netdev, davem, willemb, dborkman, edumazet, Daniel Baluta
In-Reply-To: <1365503461-26309-2-git-send-email-alex.mihai.c@gmail.com>
Hello.
On 09-04-2013 14:30, Alexandru Copot wrote:
> Signed-of by Alexandru Copot <alex.mihai.c@gmail.com>
> Cc: Daniel Baluta <dbaluta@ixiacom.com>
[...]
> diff --git a/tools/testing/selftests/net/selftests.c b/tools/testing/selftests/net/selftests.c
> new file mode 100644
> index 0000000..cd6e427
> --- /dev/null
> +++ b/tools/testing/selftests/net/selftests.c
> @@ -0,0 +1,30 @@
> +#include <stdio.h>
> +
> +#include "selftests.h"
> +
> +int run_all_tests(struct generic_test *test, void *param)
> +{
> + int i;
> + int rc, allrc;
> + char *ptr;
> +
> + rc = test->prepare ? test->prepare(param) : 0;
> + if (rc)
> + return rc;
> +
> + allrc = 0;
> + printf("Testing: %s ", test->name);
> + ptr = test->testcases;
> + for (i = 0; i < test->testcase_count; i++) {
> + rc = test->run(ptr);
> + allrc |= rc;
> +
> + if (test->abort_on_fail && rc) {
> + printf("Testcase %d failed, aborting\n", i);
> + }
Nit: {} not needed here, at least if you folow the Linux coding style (you
seem to).
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 2/2] netprio_cgroup: remove task_struct parameter from sock_update_netprio()
From: Neil Horman @ 2013-04-09 13:39 UTC (permalink / raw)
To: Li Zefan; +Cc: David Miller, netdev, LKML
In-Reply-To: <5163AF43.502@huawei.com>
On Tue, Apr 09, 2013 at 02:03:47PM +0800, Li Zefan wrote:
> The callers always pass current to sock_update_netprio().
>
> Signed-off-by: Li Zefan <lizefan@huawei.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH 4/7] xen-netfront: frags -> slots in log message
From: Sergei Shtylyov @ 2013-04-09 13:35 UTC (permalink / raw)
To: Wei Liu
Cc: netdev, xen-devel, ian.campbell, david.vrabel, konrad.wilk,
annie.li, wdauchy
In-Reply-To: <1365505655-8021-5-git-send-email-wei.liu2@citrix.com>
Hello.
On 09-04-2013 15:07, Wei Liu wrote:
> Also fix a typo in comment.
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> ---
> drivers/net/xen-netfront.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
> index d9097a7..1bb2e20 100644
> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
[...]
> @@ -771,7 +771,7 @@ next:
>
> if (unlikely(slots > max)) {
> if (net_ratelimit())
> - dev_warn(dev, "Too many frags\n");
> + dev_warn(dev, "Too many slots\n");
Shouldn't you have done this change as a part of patch #2?
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/2] cls_cgroup: remove task_struct parameter from sock_update_classid()
From: Neil Horman @ 2013-04-09 13:35 UTC (permalink / raw)
To: Li Zefan; +Cc: David Miller, netdev, LKML
In-Reply-To: <5163AF37.8010605@huawei.com>
On Tue, Apr 09, 2013 at 02:03:35PM +0800, Li Zefan wrote:
> The callers always pass current to sock_update_classid().
>
> Signed-off-by: Li Zefan <lizefan@huawei.com>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet
From: Paul Moore @ 2013-04-09 13:33 UTC (permalink / raw)
To: Casey Schaufler
Cc: Eric Dumazet, David Miller, netdev, mvadkert, selinux,
linux-security-module
In-Reply-To: <28452040.xEi3pLPik0@sifl>
On Tuesday, April 09, 2013 09:19:30 AM Paul Moore wrote:
> On Monday, April 08, 2013 06:24:59 PM Casey Schaufler wrote:
> > On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> > > On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
> > >> I don't see that with adding 4 bytes. Again, I'm willing to be
> > >> educated if I'm wrong.
> > >
> > > Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> > > on 64 bit arches. Show us your patch.
> >
> > Recall that it's replacing an existing 4 byte value with an 8 byte value.
> > My compiler days were quite short and long ago, but it would seem that
> > an 8 byte value ought not have an 'align to 8 bytes' problem.
> >
> > Again, I'm willing to be educated.
>
> Armed with a cup of coffee I took a look at the sk_buff structure this
> morning with the pahole tool and using the current sk_buff if we turn on
> all the #ifdefs here is what I see on x86_64:
>
> struct sk_buff {
...
> /* size: 280, cachelines: 5, members: 62 */
> /* sum members: 270, holes: 3, sum holes: 10 */
> /* bit holes: 1, sum bit holes: 6 bits */
> /* last cacheline: 24 bytes */
> };
>
> It looks like there some holes we might be able to capitalize on. If we
> remove "secmark" (we can handle it inside a security blob) and move
> "protocol" to after the flags2 bit field we can make an aligned 8 byte hold
> for a security blob before "destructor". According to pahole the structure
> size stays the same and the only field which moves to a different cacheline
> is "dma_cookie" which moves from cacheline 2 to 3. Here is the pahole
> output:
>
> struct sk_buff_test {
...
> /* size: 280, cachelines: 5, members: 62 */
> /* sum members: 274, holes: 3, sum holes: 6 */
> /* bit holes: 1, sum bit holes: 6 bits */
> /* last cacheline: 24 bytes */
> };
>
> As Casey already mentioned, if this isn't acceptable please help me
> understand why.
For the sake of completeness I also checked out the changes when compiled for
32 bit and it was very much the same; same structure size and in the 32 bit
case no field movement from one cacheline to another.
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [E1000-devel] [PATCH] igb: fix PHC stopping on max freq
From: Richard Cochran @ 2013-04-09 13:26 UTC (permalink / raw)
To: Jeff Kirsher
Cc: Jiri Benc, e1000-devel, netdev, Miroslav Lichvar, Stefan, Assmann
In-Reply-To: <1363740552.2069.55.camel@jtkirshe-mobl>
On Tue, Mar 19, 2013 at 05:49:12PM -0700, Jeff Kirsher wrote:
>
> Thanks Jiri, I have added the patch to my igb queue
Can this patch also go into stable, since v3.5, please?
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH] tcp: assign the sock correctly to an outgoing SYNACK packet
From: Paul Moore @ 2013-04-09 13:19 UTC (permalink / raw)
To: Casey Schaufler
Cc: Eric Dumazet, David Miller, netdev, mvadkert, selinux,
linux-security-module
In-Reply-To: <51636DEB.5000308@schaufler-ca.com>
On Monday, April 08, 2013 06:24:59 PM Casey Schaufler wrote:
> On 4/8/2013 6:09 PM, Eric Dumazet wrote:
> > On Mon, 2013-04-08 at 17:59 -0700, Casey Schaufler wrote:
> >> I don't see that with adding 4 bytes. Again, I'm willing to be
> >> educated if I'm wrong.
> >
> > Feel free to add 4 bytes without having the 'align to 8 bytes' problem
> > on 64 bit arches. Show us your patch.
>
> Recall that it's replacing an existing 4 byte value with an 8 byte value.
> My compiler days were quite short and long ago, but it would seem that
> an 8 byte value ought not have an 'align to 8 bytes' problem.
>
> Again, I'm willing to be educated.
Armed with a cup of coffee I took a look at the sk_buff structure this morning
with the pahole tool and using the current sk_buff if we turn on all the
#ifdefs here is what I see on x86_64:
struct sk_buff {
struct sk_buff * next; /* 0 8 */
struct sk_buff * prev; /* 8 8 */
ktime_t tstamp; /* 16 8 */
struct sock * sk; /* 24 8 */
struct net_device * dev; /* 32 8 */
char cb[48]; /* 40 48 */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
long unsigned int _skb_refdst; /* 88 8 */
struct sec_path * sp; /* 96 8 */
unsigned int len; /* 104 4 */
unsigned int data_len; /* 108 4 */
__u16 mac_len; /* 112 2 */
__u16 hdr_len; /* 114 2 */
union {
__wsum csum; /* 4 */
struct {
__u16 csum_start; /* 116 2 */
__u16 csum_offset; /* 118 2 */
}; /* 4 */
}; /* 116 4 */
__u32 priority; /* 120 4 */
int flags1_begin[0]; /* 124 0 */
__u8 local_df:1; /* 124: 7 1 */
__u8 cloned:1; /* 124: 6 1 */
__u8 ip_summed:2; /* 124: 4 1 */
__u8 nohdr:1; /* 124: 3 1 */
__u8 nfctinfo:3; /* 124: 0 1 */
__u8 pkt_type:3; /* 125: 5 1 */
__u8 fclone:2; /* 125: 3 1 */
__u8 ipvs_property:1; /* 125: 2 1 */
__u8 peeked:1; /* 125: 1 1 */
__u8 nf_trace:1; /* 125: 0 1 */
/* XXX 2 bytes hole, try to pack */
/* --- cacheline 2 boundary (128 bytes) --- */
int flags1_end[0]; /* 128 0 */
__be16 protocol; /* 128 2 */
/* XXX 6 bytes hole, try to pack */
void (*destructor)(struct sk_buff *); /* 136
8 */
struct nf_conntrack * nfct; /* 144 8 */
struct sk_buff * nfct_reasm; /* 152 8 */
struct nf_bridge_info * nf_bridge; /* 160 8 */
int skb_iif; /* 168 4 */
__u32 rxhash; /* 172 4 */
__u16 vlan_tci; /* 176 2 */
__u16 tc_index; /* 178 2 */
__u16 tc_verd; /* 180 2 */
__u16 queue_mapping; /* 182 2 */
int flags2_begin[0]; /* 184 0 */
__u8 ndisc_nodetype:2; /* 184: 6 1 */
__u8 pfmemalloc:1; /* 184: 5 1 */
__u8 ooo_okay:1; /* 184: 4 1 */
__u8 l4_rxhash:1; /* 184: 3 1 */
__u8 wifi_acked_valid:1; /* 184: 2 1 */
__u8 wifi_acked:1; /* 184: 1 1 */
__u8 no_fcs:1; /* 184: 0 1 */
__u8 head_frag:1; /* 185: 7 1 */
__u8 encapsulation:1; /* 185: 6 1 */
/* XXX 6 bits hole, try to pack */
/* XXX 2 bytes hole, try to pack */
int flags2_end[0]; /* 188 0 */
dma_cookie_t dma_cookie; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__u32 secmark; /* 192 4 */
union {
__u32 mark; /* 4 */
__u32 dropcount; /* 4 */
__u32 reserved_tailroom; /* 4 */
}; /* 196 4 */
sk_buff_data_t inner_transport_header; /* 200 8 */
sk_buff_data_t inner_network_header; /* 208 8 */
sk_buff_data_t transport_header; /* 216 8 */
sk_buff_data_t network_header; /* 224 8 */
sk_buff_data_t mac_header; /* 232 8 */
sk_buff_data_t tail; /* 240 8 */
sk_buff_data_t end; /* 248 8 */
/* --- cacheline 4 boundary (256 bytes) --- */
unsigned char * head; /* 256 8 */
unsigned char * data; /* 264 8 */
unsigned int truesize; /* 272 4 */
atomic_t users; /* 276 4 */
/* size: 280, cachelines: 5, members: 62 */
/* sum members: 270, holes: 3, sum holes: 10 */
/* bit holes: 1, sum bit holes: 6 bits */
/* last cacheline: 24 bytes */
};
It looks like there some holes we might be able to capitalize on. If we
remove "secmark" (we can handle it inside a security blob) and move "protocol"
to after the flags2 bit field we can make an aligned 8 byte hold for a
security blob before "destructor". According to pahole the structure size
stays the same and the only field which moves to a different cacheline is
"dma_cookie" which moves from cacheline 2 to 3. Here is the pahole output:
struct sk_buff_test {
struct sk_buff * next; /* 0 8 */
struct sk_buff * prev; /* 8 8 */
ktime_t tstamp; /* 16 8 */
struct sock * sk; /* 24 8 */
struct net_device * dev; /* 32 8 */
char cb[48]; /* 40 48 */
/* --- cacheline 1 boundary (64 bytes) was 24 bytes ago --- */
long unsigned int _skb_refdst; /* 88 8 */
struct sec_path * sp; /* 96 8 */
unsigned int len; /* 104 4 */
unsigned int data_len; /* 108 4 */
__u16 mac_len; /* 112 2 */
__u16 hdr_len; /* 114 2 */
union {
__wsum csum; /* 4 */
struct {
__u16 csum_start; /* 116 2 */
__u16 csum_offset; /* 118 2 */
}; /* 4 */
}; /* 116 4 */
__u32 priority; /* 120 4 */
int flags1_begin[0]; /* 124 0 */
__u8 local_df:1; /* 124: 7 1 */
__u8 cloned:1; /* 124: 6 1 */
__u8 ip_summed:2; /* 124: 4 1 */
__u8 nohdr:1; /* 124: 3 1 */
__u8 nfctinfo:3; /* 124: 0 1 */
__u8 pkt_type:3; /* 125: 5 1 */
__u8 fclone:2; /* 125: 3 1 */
__u8 ipvs_property:1; /* 125: 2 1 */
__u8 peeked:1; /* 125: 1 1 */
__u8 nf_trace:1; /* 125: 0 1 */
/* XXX 2 bytes hole, try to pack */
/* --- cacheline 2 boundary (128 bytes) --- */
int flags1_end[0]; /* 128 0 */
void * security; /* 128 8 */
void (*destructor)(struct sk_buff *); /* 136
8 */
struct nf_conntrack * nfct; /* 144 8 */
struct sk_buff * nfct_reasm; /* 152 8 */
struct nf_bridge_info * nf_bridge; /* 160 8 */
int skb_iif; /* 168 4 */
__u32 rxhash; /* 172 4 */
__u16 vlan_tci; /* 176 2 */
__u16 tc_index; /* 178 2 */
__u16 tc_verd; /* 180 2 */
__u16 queue_mapping; /* 182 2 */
int flags2_begin[0]; /* 184 0 */
__u8 ndisc_nodetype:2; /* 184: 6 1 */
__u8 pfmemalloc:1; /* 184: 5 1 */
__u8 ooo_okay:1; /* 184: 4 1 */
__u8 l4_rxhash:1; /* 184: 3 1 */
__u8 wifi_acked_valid:1; /* 184: 2 1 */
__u8 wifi_acked:1; /* 184: 1 1 */
__u8 no_fcs:1; /* 184: 0 1 */
__u8 head_frag:1; /* 185: 7 1 */
__u8 encapsulation:1; /* 185: 6 1 */
/* XXX 6 bits hole, try to pack */
/* XXX 2 bytes hole, try to pack */
int flags2_end[0]; /* 188 0 */
__be16 protocol; /* 188 2 */
/* XXX 2 bytes hole, try to pack */
/* --- cacheline 3 boundary (192 bytes) --- */
dma_cookie_t dma_cookie; /* 192 4 */
union {
__u32 mark; /* 4 */
__u32 dropcount; /* 4 */
__u32 reserved_tailroom; /* 4 */
}; /* 196 4 */
sk_buff_data_t inner_transport_header; /* 200 8 */
sk_buff_data_t inner_network_header; /* 208 8 */
sk_buff_data_t transport_header; /* 216 8 */
sk_buff_data_t network_header; /* 224 8 */
sk_buff_data_t mac_header; /* 232 8 */
sk_buff_data_t tail; /* 240 8 */
sk_buff_data_t end; /* 248 8 */
/* --- cacheline 4 boundary (256 bytes) --- */
unsigned char * head; /* 256 8 */
unsigned char * data; /* 264 8 */
unsigned int truesize; /* 272 4 */
atomic_t users; /* 276 4 */
/* size: 280, cachelines: 5, members: 62 */
/* sum members: 274, holes: 3, sum holes: 6 */
/* bit holes: 1, sum bit holes: 6 bits */
/* last cacheline: 24 bytes */
};
As Casey already mentioned, if this isn't acceptable please help me understand
why.
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [RFC] net : add tx timestamp to packet mmap.
From: Richard Cochran @ 2013-04-09 13:15 UTC (permalink / raw)
To: Paul Chavent; +Cc: davem, edumazet, daniel.borkmann, xemul, ebiederm, netdev
In-Reply-To: <5163F0B1.7090309@onera.fr>
On Tue, Apr 09, 2013 at 12:42:57PM +0200, Paul Chavent wrote:
>
> Would it be possible that the packet mmap maintainers give their
> opinion on this thread please ?
I was digging around, trying to understand whether libpcap can get HW
time stamps via the packet_mmap interface, and I found this.
commit 614f60fa9d73a9e8fdff3df83381907fea7c5649
Author: Scott McMillan <scott.a.mcmillan@intel.com>
Date: Wed Jun 2 05:53:56 2010 -0700
packet_mmap: expose hw packet timestamps to network packet capture utilities
Maybe you could ask Scott for help?
[ It looks to me like that patch is kinda useless, since the user has
no way to tell whether the time stamps are from HW or SW. ]
HTH,
Richard
^ permalink raw reply
* Re: [Xen-devel] [PATCH 6/7] xen-netback: coalesce slots and fix regressions
From: Jan Beulich @ 2013-04-09 13:13 UTC (permalink / raw)
To: Wei Liu
Cc: David Vrabel, IanCampbell, wdauchy@gmail.com,
xen-devel@lists.xen.org, annie.li@oracle.com,
konrad.wilk@oracle.com, netdev@vger.kernel.org
In-Reply-To: <20130409124845.GG16998@zion.uk.xensource.com>
>>> On 09.04.13 at 14:48, Wei Liu <wei.liu2@citrix.com> wrote:
> On Tue, Apr 09, 2013 at 01:13:39PM +0100, Jan Beulich wrote:
>> >>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
> [...]
>> > +
>> > +static struct kernel_param_ops max_skb_slots_param_ops = {
>>
>> __moduleparam_const
>
> TBH I don't see any driver makes use of this.
Sure, because generally you use the simple module_param() or
module_param_named() macros.
> Probably a simple "const" can do?
The purpose of __moduleparam_const is to abstract away the
need to not have the const for a very limited set of architectures.
Even if Xen currently doesn't support any of those, I would still
not want to see architecture incompatibilities introduced if
avoidable.
>> > @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
>> > int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
>> >
>> > if (vif->can_sg || vif->gso || vif->gso_prefix)
>> > - max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
>> > + max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
>> >
>> > return max;
>> > }
>> > @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>> > __skb_queue_tail(&rxq, skb);
>> >
>> > /* Filled the batch queue? */
>> > - if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
>> > + if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
>> > break;
>> > }
>> >
>>
>> Are the two changes above really correct? You're having an skb as
>> input here, and hence you want to use all the frags, and nothing
>> beyond. Another question is whether the frontend can handle
>> those, but that aspect isn't affected by the code being modified
>> here.
>>
>
> This patch tries to remove dependency on MAX_SKB_FRAGS. Writing the
> protocol-defined value here is OK IMHO.
I understand the intentions of the patch, but you shouldn't go
further with this than you need to. Just think through carefully
the cases of MAX_SKB_FRAGS being smaller/bigger than
XEN_NETIF_NR_SLOTS_MIN: In the first instance, you needlessly
return too big a value when the latter is the bigger one, and in
the second instance you bail from the loop early in the same case.
What's worse, in the opposite case I'm having the impression that
you would continue the loop when you shouldn't (because there's
not enough room left), and I'd suspect problems for the caller of
max_required_rx_slots() in that case too.
>> > return -E2BIG;
>> > }
>> >
>> > - memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
>> > + memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
>> > sizeof(*txp));
>> > if (txp->size > first->size) {
>> > - netdev_err(vif->dev, "Frag is bigger than frame.\n");
>> > + netdev_err(vif->dev, "Packet is bigger than frame.\n");
>>
>> I don't think "packet" is the right term here.
>>
>
> Maybe just "Invalid tx request" and dump all information?
Possibly. But this may become a little too verbose then...
Jan
^ permalink raw reply
* Re: [PATCH 2/2] sh_eth: add R-Car support for real
From: Sergei Shtylyov @ 2013-04-09 13:08 UTC (permalink / raw)
To: phil.edworthy; +Cc: Simon Horman, linux-sh, netdev, nobuhiro.iwamatsu.yj
In-Reply-To: <OF927F8D41.407FE3E6-ON80257B47.004FDFDA-80257B47.005090A7@eu.necel.com>
Hello.
On 08-04-2013 18:40, phil.edworthy@renesas.com wrote:
> Thanks for the patch. When I added r8a7779 support, I had a rather poor
> manual with a lot of information missing. I submitted the patch once I got
> the driver working on a Marzen board, with an REE Expansion Board.
That must be 3rd type of the expansion board even. I have documentation on
some Y-R-CAR-H1-BOARD-EXP board, and I have the R0P7779A00020S board mounted
below Marzen. They seem very different as the latter don't have any switches
on it.
> One of the main changes you have made is adjusting the offset of the
> control registers. When I did this work originally, I just assumed that
> the base address start at 0x1DE00200, i.e. it included the 0x200 offset.
> This made sense as the registers are then identically positioned to sh4a.
Yes, but not all SH4 registers exist on R-Car and the Ether base is
0xFDE00000 according to the datasheet.
> Regards
> Phil
WBR, Sergei
^ permalink raw reply
* Re: [PATCH 1/3] if.h: add IFF_BRIDGE_RESTRICTED flag
From: Jamal Hadi Salim @ 2013-04-09 12:57 UTC (permalink / raw)
To: Antonio Quartulli
Cc: Stephen Hemminger, David S. Miller,
bridge@lists.linux-foundation.org, netdev@vger.kernel.org
In-Reply-To: <20130409075606.GB3771@open-mesh.com>
Hi,
Consider using tc for this.
You can tag the packet using skb mark on the receiving end point,
match them on the bridge and execute actions not to forward them.
cheers,
jamal
On 13-04-09 03:56 AM, Antonio Quartulli wrote:
> On Mon, Apr 08, 2013 at 11:58:48 -0700, Stephen Hemminger wrote:
>> The standard way to do this is to use netfilter. Considering the
>> additional device flags and skb flag changes, I am not sure that your
>> method is better.
>
> To make it a bit more clear:
>
> 1) the skb flag will be used on the "receiving end-point" by batman-adv to mark
> received packets and so instruct the bridge to do not forward them to restricted
> interfaces.
>
> 2) the IFF_ flag is used by batman-adv on the "sending side" to determine
> whether a packet has been originated by a restricted interface and so instruct
> the remote endpoint to mark the skb when received.
>
> 3) to make the bridge code general enough, I decided to let it mark packets
> coming from restricted interfaces as well so that it can also apply the policy
> at 1) locally, without any further setting. The logic described in 1) is
> therefore applied by the bridge even for local packets (not passing through
> batman-adv)
>
>
>
> Point 3) is the only one where netfilter might help. But using two mechanism to
> achieve one goal looked not sane to me and therefore I decided to to do it this
> way. And actually the code allowing point 3 is only:
>
> + skb->bridge_restricted = !!(skb->dev->flags & IFF_BRIDGE_RESTRICTED);
>
>
> I hope this summary did not create further confusion :)
>
^ permalink raw reply
* Re: [Xen-devel] [PATCH 6/7] xen-netback: coalesce slots and fix regressions
From: Wei Liu @ 2013-04-09 12:48 UTC (permalink / raw)
To: Jan Beulich
Cc: Wei Liu, David Vrabel, Ian Campbell, wdauchy@gmail.com,
xen-devel@lists.xen.org, annie.li@oracle.com,
konrad.wilk@oracle.com, netdev@vger.kernel.org
In-Reply-To: <5164221302000078000CBBF2@nat28.tlf.novell.com>
On Tue, Apr 09, 2013 at 01:13:39PM +0100, Jan Beulich wrote:
> >>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
[...]
> > +
> > +static struct kernel_param_ops max_skb_slots_param_ops = {
>
> __moduleparam_const
TBH I don't see any driver makes use of this. Probably a simple "const"
can do?
>
> > + .set = max_skb_slots_set,
> > + .get = param_get_ulong,
>
> param_get_uint
>
Done.
> > @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
> > int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
> >
> > if (vif->can_sg || vif->gso || vif->gso_prefix)
> > - max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
> > + max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
> >
> > return max;
> > }
> > @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
> > __skb_queue_tail(&rxq, skb);
> >
> > /* Filled the batch queue? */
> > - if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> > + if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
> > break;
> > }
> >
>
> Are the two changes above really correct? You're having an skb as
> input here, and hence you want to use all the frags, and nothing
> beyond. Another question is whether the frontend can handle
> those, but that aspect isn't affected by the code being modified
> here.
>
This patch tries to remove dependency on MAX_SKB_FRAGS. Writing the
protocol-defined value here is OK IMHO.
[...]
> > return -E2BIG;
> > }
> >
> > - memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
> > + memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> > sizeof(*txp));
> > if (txp->size > first->size) {
> > - netdev_err(vif->dev, "Frag is bigger than frame.\n");
> > + netdev_err(vif->dev, "Packet is bigger than frame.\n");
>
> I don't think "packet" is the right term here.
>
Maybe just "Invalid tx request" and dump all information?
Wei.
> Jan
^ permalink raw reply
* Re: [RFC PATCH ipsec] xfrm: use the right dev to fill xdst
From: Steffen Klassert @ 2013-04-09 12:47 UTC (permalink / raw)
To: Daniel Baluta; +Cc: Nicolas Dichtel, herbert, davem, netdev
In-Reply-To: <CAEnQRZDUjaL56AggThN50sYJpA8diQdQ85D4xCqpwtv8PbR7mg@mail.gmail.com>
On Fri, Apr 05, 2013 at 03:59:59PM +0300, Daniel Baluta wrote:
> On Fri, Apr 5, 2013 at 12:46 PM, Steffen Klassert
> <steffen.klassert@secunet.com> wrote:
> > On Thu, Apr 04, 2013 at 05:12:42PM +0200, Nicolas Dichtel wrote:
> >> Commit bc8e4b954e46 (xfrm6: ensure to use the same dev when building a bundle)
> >> broke IPsec for IPv4 over IPv6 tunnels (because dev points to an IPv4 only
> >> interface, hence in6_dev_get(dev) returns NULL.
> >
> > Can you give some informations on how to reproduce this? I'm running
> > interfamily tunnels on our testing environment and it seems to
> > work fine.
>
> I can hit this in our setup while using some internal custom simulated
> interfaces.
>
> Anyhow, this should be reproducible with a classic IPv6 IPsec over
> IPv4 test. Please make sure
> that the IPv4 interface doesn't have an IPv6 address set up.
>
> Quoting from commit bc8e4b954e46 (xfrm6: ensure to use the same dev
> when building a bundle):
>
> - xdst->u.rt6.rt6i_idev = in6_dev_get(rt->u.dst.dev);
> + xdst->u.rt6.rt6i_idev = in6_dev_get(dev);
>
> dev points to IPv4 endpoint and if it doesn't have an IPv6 address
> associated then
> in6_dev_get(dev) will return NULL.
Hm, inet6_init() registers addrconf_notify() as a netdevice notifier
function. So addrconf_notify() is called whenever a netdevice is
registered. When looking at addrconf_notify(), there are only two
cases when the net_device has no inet6_dev assigned. This is either
on error, or if the device mtu is smaller than IPV6_MIN_MTU (i.e. 1280).
I can reproduce the behaviour you describe if I set the mtu of the
ipv4 device to a value below IPV6_MIN_MTU, but in no other case.
Is it possible that your ipv4 device has a mtu below IPV6_MIN_MTU?
^ permalink raw reply
* Re: [Xen-devel] [PATCH 6/7] xen-netback: coalesce slots and fix regressions
From: Jan Beulich @ 2013-04-09 12:13 UTC (permalink / raw)
To: Wei Liu
Cc: david.vrabel, ian.campbell, wdauchy, xen-devel, annie.li,
konrad.wilk, netdev
In-Reply-To: <1365505655-8021-7-git-send-email-wei.liu2@citrix.com>
>>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@citrix.com> wrote:
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -47,11 +47,47 @@
> #include <asm/xen/hypercall.h>
> #include <asm/xen/page.h>
>
> +/*
> + * This is the maximum slots a skb can have. If a guest sends a skb
> + * which exceeds this limit it is considered malicious.
> + */
> +#define MAX_SKB_SLOTS_DEFAULT 20
> +static unsigned int max_skb_slots = MAX_SKB_SLOTS_DEFAULT;
> +
> +static int max_skb_slots_set(const char *val, const struct kernel_param *kp)
> +{
> + int ret;
> + unsigned long param = 0;
> +
> + ret = kstrtoul(val, 10, ¶m);
> +
> + if (ret < 0 || param < XEN_NETIF_NR_SLOTS_MIN)
> + return -EINVAL;
> +
> + max_skb_slots = param;
> +
> + return 0;
> +}
> +
> +static struct kernel_param_ops max_skb_slots_param_ops = {
__moduleparam_const
> + .set = max_skb_slots_set,
> + .get = param_get_ulong,
param_get_uint
> @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
> int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
>
> if (vif->can_sg || vif->gso || vif->gso_prefix)
> - max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
> + max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
>
> return max;
> }
> @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
> __skb_queue_tail(&rxq, skb);
>
> /* Filled the batch queue? */
> - if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> + if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
> break;
> }
>
Are the two changes above really correct? You're having an skb as
input here, and hence you want to use all the frags, and nothing
beyond. Another question is whether the frontend can handle
those, but that aspect isn't affected by the code being modified
here.
> @@ -904,38 +944,56 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
>
> static int netbk_count_requests(struct xenvif *vif,
> struct xen_netif_tx_request *first,
> + RING_IDX first_idx,
> struct xen_netif_tx_request *txp,
> int work_to_do)
> {
> RING_IDX cons = vif->tx.req_cons;
> - int frags = 0;
> + int slots = 0;
> + int drop_err = 0;
>
> if (!(first->flags & XEN_NETTXF_more_data))
> return 0;
>
> do {
> - if (frags >= work_to_do) {
> - netdev_err(vif->dev, "Need more frags\n");
> + if (slots >= work_to_do) {
> + netdev_err(vif->dev, "Need more slots\n");
> netbk_fatal_tx_err(vif);
> return -ENODATA;
> }
>
> - if (unlikely(frags >= MAX_SKB_FRAGS)) {
> - netdev_err(vif->dev, "Too many frags\n");
> + /* Xen network protocol had implicit dependency on
> + * MAX_SKB_FRAGS. XEN_NETIF_NR_SLOTS_MIN is set to the
> + * historical MAX_SKB_FRAGS value 18 to honor the same
> + * behavior as before. Any packet using more than 18
> + * slots but less than max_skb_slots slots is dropped
> + */
> + if (!drop_err && slots >= XEN_NETIF_NR_SLOTS_MIN) {
> + if (net_ratelimit())
> + netdev_dbg(vif->dev, "Too many slots\n");
> + drop_err = -E2BIG;
> + }
> +
> + /* This guest is really using too many slots and
> + * considered malicious.
> + */
> + if (unlikely(slots >= max_skb_slots)) {
> + netdev_err(vif->dev,
> + "Malicious frontend using too many slots\n");
Wouldn't you better swap this and the previous if?
> netbk_fatal_tx_err(vif);
> return -E2BIG;
> }
>
> - memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
> + memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
> sizeof(*txp));
> if (txp->size > first->size) {
> - netdev_err(vif->dev, "Frag is bigger than frame.\n");
> + netdev_err(vif->dev, "Packet is bigger than frame.\n");
I don't think "packet" is the right term here.
Jan
^ permalink raw reply
* Re: [PATCH] selinux: add a skb_owned_by() hook
From: Paul Moore @ 2013-04-09 12:06 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev, mvadkert, linux-security-module
In-Reply-To: <1365479891.3887.99.camel@edumazet-glaptop>
On Monday, April 08, 2013 08:58:11 PM Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> Commit 90ba9b1986b5ac (tcp: tcp_make_synack() can use alloc_skb())
> broke certain SELinux/NetLabel configurations by no longer correctly
> assigning the sock to the outgoing SYNACK packet.
>
> Cost of atomic operations on the LISTEN socket is quite big,
> and we would like it to happen only if really needed.
>
> This patch introduces a new security_ops->skb_owned_by() method,
> that is a void operation unless selinux is active.
>
> Reported-by: Miroslav Vadkerti <mvadkert@redhat.com>
> Diagnosed-by: Paul Moore <pmoore@redhat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-security-module@vger.kernel.org
> ---
> include/linux/security.h | 8 ++++++++
> net/ipv4/tcp_output.c | 1 +
> security/capability.c | 6 ++++++
> security/security.c | 5 +++++
> security/selinux/hooks.c | 7 +++++++
> 5 files changed, 27 insertions(+)
I've already voiced my objections to this approach, but I've just tested it
and it does resolve the regression in the network stack.
Tested-by: Paul Moore <pmoore@redhat.com>
Acked-by: Paul Moore <pmoore@redhat.com>
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [PATCH 7/7] xen-netback: don't disconnect frontend when seeing oversize packet
From: Wei Liu @ 2013-04-09 11:54 UTC (permalink / raw)
To: David Laight
Cc: Wei Liu, netdev@vger.kernel.org, xen-devel@lists.xen.org,
Ian Campbell, David Vrabel, konrad.wilk@oracle.com,
annie.li@oracle.com, wdauchy@gmail.com
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B71C5@saturn3.aculab.com>
On Tue, Apr 09, 2013 at 12:34:42PM +0100, David Laight wrote:
> > + if (!drop_err && txp->size > first->size) {
> > + if (net_ratelimit())
> > + netdev_dbg(vif->dev,
> > + "Packet is bigger than frame.\n");
>
> It must be worth printing txp->size and first->size here.
> Similarly for the other errors in the other patches.
>
Sure.
> Probably difficult for some of these errors, but it is
> sometimes worth saving the last 'bad' item. So that with
> an appropriate tool (maybe hexdump of /dev/kmem) it is
> possible to look at the actual contents and thus determine
> the actual source of the error.
>
I doubt that you can get much information by analysing txp, it is just
controll structure in ring, not the actual packet content. The packet is
still in DomU.
Wei.
> David
>
^ permalink raw reply
* Re: [PATCH] selinux: add a skb_owned_by() hook
From: Paul Moore @ 2013-04-09 11:45 UTC (permalink / raw)
To: Casey Schaufler
Cc: Eric Dumazet, David Miller, netdev, mvadkert,
linux-security-module
In-Reply-To: <5163992F.30406@schaufler-ca.com>
On Monday, April 08, 2013 09:29:35 PM Casey Schaufler wrote:
> Does it affect Smack (which uses NetLabel) as well?
The short answer is "no". Smack approaches the network access controls
differently and as a result has hooks in different places that do slightly
different things; the regression in the network stack only affects SELinux.
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* Re: [PATCH] selinux: add a skb_owned_by() hook
From: Paul Moore @ 2013-04-09 11:39 UTC (permalink / raw)
To: David Miller, casey; +Cc: eric.dumazet, netdev, mvadkert, linux-security-module
In-Reply-To: <20130409.004144.1226810973846202358.davem@davemloft.net>
On Tuesday, April 09, 2013 12:41:44 AM David Miller wrote:
> It makes sure SYN/ACKs have a socket context attached to the
> packet, which only LSMs actually need.
>
> You participated in the thread where this stuff was discussed and the
> initial version of this patch was posted, so this patch, or any aspect
> of it, should not be a mystery.
Casey, and the LSM list as a whole, was not included in the entire thread as
when I first posted my original patch I believed the "mergeable" fix was going
to be self contained within the network stack.
For Casey, and others on the LSM list, here is a pointer to the original
thread which started on netdev. Enjoy.
* http://marc.info/?t=136543607500006&r=1&w=2
--
paul moore
security and virtualization @ redhat
^ permalink raw reply
* RE: [PATCH 7/7] xen-netback: don't disconnect frontend when seeing oversize packet
From: David Laight @ 2013-04-09 11:34 UTC (permalink / raw)
To: Wei Liu, netdev, xen-devel
Cc: ian.campbell, david.vrabel, konrad.wilk, annie.li, wdauchy
In-Reply-To: <1365505655-8021-8-git-send-email-wei.liu2@citrix.com>
> + if (!drop_err && txp->size > first->size) {
> + if (net_ratelimit())
> + netdev_dbg(vif->dev,
> + "Packet is bigger than frame.\n");
It must be worth printing txp->size and first->size here.
Similarly for the other errors in the other patches.
Probably difficult for some of these errors, but it is
sometimes worth saving the last 'bad' item. So that with
an appropriate tool (maybe hexdump of /dev/kmem) it is
possible to look at the actual contents and thus determine
the actual source of the error.
David
^ permalink raw reply
* Re: [PATCH 1/3 net-next RFC] selftest: add abstractions for net selftests
From: Daniel Borkmann @ 2013-04-09 11:32 UTC (permalink / raw)
To: Alexandru Copot; +Cc: netdev, davem, willemb, edumazet, Daniel Baluta
In-Reply-To: <CAHG7+CBWF-gLcNfvnP2NQjkAwgtv+k6=0DndN1Mu2b+1acHj-w@mail.gmail.com>
On 04/09/2013 01:24 PM, Alexandru Copot wrote:
> On Tue, Apr 9, 2013 at 2:13 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
>>> +#define CHECK(cond,fmt,...) \
>>> + do { \
>>> + if (!(cond)) { \
>>> + fprintf(stderr, "(%s, %d): " fmt, \
>>> + __FILE__, __LINE__,
>>> ##__VA_ARGS__); \
>>> + perror(""); \
>>> + return 1; \
>>> + } \
>>> + } while (0)
>>
>>
>> Isn't it a bit error-prone if in the middle of somewhere this check fails
>> and the function suddenly returns 1?
>>
>> What if this is called from a function that was declared as void or to
>> return a pointer to a struct etc.?
>
> Well, I tought of using this only in your high-level testcase methods
> (test->run()).
> It is also easier to see what is actually being tested.
>
> For anything else the user is free to use any other functions or
> return conventions
> as the test requires.
Hm, then, still not convinced about the CHECK macro. In worst case this at
least needs a comment, so that people will not misuse that, but with your
two statements above, it seems likely that people could also start using it
in "any other functions or return conventions as the test requires". ;-)
^ permalink raw reply
* Re: [PATCH 1/3 net-next RFC] selftest: add abstractions for net selftests
From: Alexandru Copot @ 2013-04-09 11:24 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, davem, willemb, edumazet, Daniel Baluta
In-Reply-To: <5163F7E1.8040208@redhat.com>
On Tue, Apr 9, 2013 at 2:13 PM, Daniel Borkmann <dborkman@redhat.com> wrote:
>> + for (i = 0; i < test->testcase_count; i++) {
>> + rc = test->run(ptr);
>> + allrc |= rc;
>> +
>> + if (test->abort_on_fail && rc) {
>> + printf("Testcase %d failed, aborting\n", i);
>> + }
>
>
> I think here you wanted to abort but didn't?
Yes, I forgot to break;
>> +#define CHECK(cond,fmt,...) \
>> + do { \
>> + if (!(cond)) { \
>> + fprintf(stderr, "(%s, %d): " fmt, \
>> + __FILE__, __LINE__,
>> ##__VA_ARGS__); \
>> + perror(""); \
>> + return 1; \
>> + } \
>> + } while (0)
>
>
> Isn't it a bit error-prone if in the middle of somewhere this check fails
> and the function suddenly returns 1?
>
> What if this is called from a function that was declared as void or to
> return a pointer to a struct etc.?
Well, I tought of using this only in your high-level testcase methods
(test->run()).
It is also easier to see what is actually being tested.
For anything else the user is free to use any other functions or
return conventions
as the test requires.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox