| Message ID | pvmhbrzboyb.fsf@chavey.mtv.corp.google.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
From: chavey@google.com Date: Wed, 09 Dec 2009 17:25:00 -0800 > Fix compiler warning "discards qualifiers from pointer target type". > The function prototype defines parameters as pointer to a constant. > Such parameters should not have their content modified in the > function. > > Signed-off-by: Laurent Chavey <chavey@google.com> Please CC: netfilter patches to netfilter-devel, added... > --- > net/ipv4/netfilter/ipt_ULOG.c | 6 ++++-- > net/netfilter/xt_time.c | 6 ++++-- > 2 files changed, 8 insertions(+), 4 deletions(-) > > diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c > index d32cc4b..8490f9f 100644 > --- a/net/ipv4/netfilter/ipt_ULOG.c > +++ b/net/ipv4/netfilter/ipt_ULOG.c > @@ -166,6 +166,7 @@ static void ipt_ulog_packet(unsigned int hooknum, > size_t size, copy_len; > struct nlmsghdr *nlh; > struct timeval tv; > + ktime_t tstamp; > > /* ffs == find first bit set, necessary because userspace > * is already shifting groupnumber, but we need unshifted. > @@ -208,13 +209,14 @@ static void ipt_ulog_packet(unsigned int hooknum, > > pm = NLMSG_DATA(nlh); > > + tstamp = skb->tstamp; > /* We might not have a timestamp, get one */ > if (skb->tstamp.tv64 == 0) > - __net_timestamp((struct sk_buff *)skb); > + tstamp = ktime_get_real(); > > /* copy hook, prefix, timestamp, payload, etc. */ > pm->data_len = copy_len; > - tv = ktime_to_timeval(skb->tstamp); > + tv = ktime_to_timeval(tstamp); > put_unaligned(tv.tv_sec, &pm->timestamp_sec); > put_unaligned(tv.tv_usec, &pm->timestamp_usec); > put_unaligned(skb->mark, &pm->mark); > diff --git a/net/netfilter/xt_time.c b/net/netfilter/xt_time.c > index 93acaa5..66e2a75 100644 > --- a/net/netfilter/xt_time.c > +++ b/net/netfilter/xt_time.c > @@ -159,6 +159,7 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par) > unsigned int packet_time; > struct xtm current_time; > s64 stamp; > + ktime_t tstamp; > > /* > * We cannot use get_seconds() instead of __net_timestamp() here. > @@ -169,10 +170,11 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par) > * may happen that the same packet matches both rules if > * it arrived at the right moment before 13:00. > */ > + tstamp = skb->tstamp; > if (skb->tstamp.tv64 == 0) > - __net_timestamp((struct sk_buff *)skb); > + tstamp = ktime_get_real(); > > - stamp = ktime_to_ns(skb->tstamp); > + stamp = ktime_to_ns(tstamp); > stamp = div_s64(stamp, NSEC_PER_SEC); > > if (info->flags & XT_TIME_LOCAL_TZ) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 10/12/2009 02:25, chavey@google.com a écrit : > Fix compiler warning "discards qualifiers from pointer target type". > The function prototype defines parameters as pointer to a constant. > Such parameters should not have their content modified in the > function. > > Signed-off-by: Laurent Chavey <chavey@google.com> This is not the right fix IMHO. We want an unique timestamp for the whole netfilter matches, because several 'time' rules could get 'interesting' effects. The 'const' attribute is a debugging aid, and the skb->tstamp 'write-once' is a valid exception. Read again the comment in time_mt() : vi +163 net/netfilter/xt_time.c static bool time_mt(const struct sk_buff *skb, const struct xt_match_param *par) { const struct xt_time_info *info = par->matchinfo; unsigned int packet_time; struct xtm current_time; s64 stamp; /* * We cannot use get_seconds() instead of __net_timestamp() here. * Suppose you have two rules: * 1. match before 13:00 * 2. match after 13:00 * If you match against processing time (get_seconds) it * may happen that the same packet matches both rules if * it arrived at the right moment before 13:00. */ if (skb->tstamp.tv64 == 0) __net_timestamp((struct sk_buff *)skb); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Dumazet wrote: > Le 10/12/2009 02:25, chavey@google.com a écrit : >> Fix compiler warning "discards qualifiers from pointer target type". >> The function prototype defines parameters as pointer to a constant. >> Such parameters should not have their content modified in the >> function. >> >> Signed-off-by: Laurent Chavey <chavey@google.com> > > This is not the right fix IMHO. > > We want an unique timestamp for the whole netfilter matches, because several 'time' rules > could get 'interesting' effects. > > The 'const' attribute is a debugging aid, and the skb->tstamp 'write-once' is a valid exception. I agree, the timestamps should be consistent among multiple matches. The cast should also surpress the warning, but some gcc versions ignore it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 9, 2009 at 8:25 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le 10/12/2009 02:25, chavey@google.com a écrit : >> Fix compiler warning "discards qualifiers from pointer target type". >> The function prototype defines parameters as pointer to a constant. >> Such parameters should not have their content modified in the >> function. >> >> Signed-off-by: Laurent Chavey <chavey@google.com> > > This is not the right fix IMHO. > > We want an unique timestamp for the whole netfilter matches, because several 'time' rules > could get 'interesting' effects. > > The 'const' attribute is a debugging aid, and the skb->tstamp 'write-once' is a valid exception. > > Read again the comment in time_mt() : good point. I agree with the need for the exception, I would just like it to be more explicit in the code itself (like a turn off check around that particular statement) so we do not have to scrub thru the compiler output to filter out good / bad warning. question: why do we not force the timestamp in the skb before going thru the chain ? it looks to me that the check for (skb->tstamp.tv64 == 0) should be done once > > vi +163 net/netfilter/xt_time.c > > static bool > time_mt(const struct sk_buff *skb, const struct xt_match_param *par) > { > const struct xt_time_info *info = par->matchinfo; > unsigned int packet_time; > struct xtm current_time; > s64 stamp; > > /* > * We cannot use get_seconds() instead of __net_timestamp() here. > * Suppose you have two rules: > * 1. match before 13:00 > * 2. match after 13:00 > * If you match against processing time (get_seconds) it > * may happen that the same packet matches both rules if > * it arrived at the right moment before 13:00. > */ > if (skb->tstamp.tv64 == 0) > __net_timestamp((struct sk_buff *)skb); > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Donnerstag 2009-12-10 18:50, Laurent Chavey wrote: > >good point. I agree with the need for the exception, I would just >like it to be more explicit in the code itself (like a turn off >check around that particular statement) so we do not have to scrub >thru the compiler output to filter out good / bad warning. question: > why do we not force the timestamp in the skb before going thru the >chain ? Because it is probably too expensive to do, unless you employ things like xt_time? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 10/12/2009 19:53, Jan Engelhardt a écrit : > > On Donnerstag 2009-12-10 18:50, Laurent Chavey wrote: >> >> good point. I agree with the need for the exception, I would just >> like it to be more explicit in the code itself (like a turn off >> check around that particular statement) so we do not have to scrub >> thru the compiler output to filter out good / bad warning. question: >> why do we not force the timestamp in the skb before going thru the >> chain ? > > Because it is probably too expensive to do, unless you employ > things like xt_time? Right. On some platforms, timestamps are quite expensive (say, a fraction of one micro second to get one of them), we try to avoid them as much as possible. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/netfilter/ipt_ULOG.c b/net/ipv4/netfilter/ipt_ULOG.c index d32cc4b..8490f9f 100644 --- a/net/ipv4/netfilter/ipt_ULOG.c +++ b/net/ipv4/netfilter/ipt_ULOG.c @@ -166,6 +166,7 @@ static void ipt_ulog_packet(unsigned int hooknum, size_t size, copy_len; struct nlmsghdr *nlh; struct timeval tv; + ktime_t tstamp; /* ffs == find first bit set, necessary because userspace * is already shifting groupnumber, but we need unshifted. @@ -208,13 +209,14 @@ static void ipt_ulog_packet(unsigned int hooknum, pm = NLMSG_DATA(nlh); + tstamp = skb->tstamp; /* We might not have a timestamp, get one */ if (skb->tstamp.tv64 == 0) - __net_timestamp((struct sk_buff *)skb); + tstamp = ktime_get_real(); /* copy hook, prefix, timestamp, payload, etc. */ pm->data_len = copy_len; - tv = ktime_to_timeval(skb->tstamp); + tv = ktime_to_timeval(tstamp); put_unaligned(tv.tv_sec, &pm->timestamp_sec); put_unaligned(tv.tv_usec, &pm->timestamp_usec); put_unaligned(skb->mark, &pm->mark); diff --git a/net/netfilter/xt_time.c b/net/netfilter/xt_time.c index 93acaa5..66e2a75 100644 --- a/net/netfilter/xt_time.c +++ b/net/netfilter/xt_time.c @@ -159,6 +159,7 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par) unsigned int packet_time; struct xtm current_time; s64 stamp; + ktime_t tstamp; /* * We cannot use get_seconds() instead of __net_timestamp() here. @@ -169,10 +170,11 @@ time_mt(const struct sk_buff *skb, const struct xt_match_param *par) * may happen that the same packet matches both rules if * it arrived at the right moment before 13:00. */ + tstamp = skb->tstamp; if (skb->tstamp.tv64 == 0) - __net_timestamp((struct sk_buff *)skb); + tstamp = ktime_get_real(); - stamp = ktime_to_ns(skb->tstamp); + stamp = ktime_to_ns(tstamp); stamp = div_s64(stamp, NSEC_PER_SEC); if (info->flags & XT_TIME_LOCAL_TZ)
Fix compiler warning "discards qualifiers from pointer target type". The function prototype defines parameters as pointer to a constant. Such parameters should not have their content modified in the function. Signed-off-by: Laurent Chavey <chavey@google.com> --- net/ipv4/netfilter/ipt_ULOG.c | 6 ++++-- net/netfilter/xt_time.c | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html