| Message ID | 1467389503-27143-1-git-send-email-carlosfg@riseup.net |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
On Fri, Jul 01, 2016 at 06:11:43PM +0200, Carlos Falgueras García wrote: > These lengths must be one character longer to take account the null > character Please, place the change for src/set.c in this patch so I only need to apply one patch. Another comment below. > Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net> > --- > src/chain.c | 2 +- > src/rule.c | 2 +- > src/set_elem.c | 2 +- > src/table.c | 2 +- > src/trace.c | 6 +++--- > 5 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/src/chain.c b/src/chain.c > index bfffbe0..cab64b5 100644 > --- a/src/chain.c > +++ b/src/chain.c > @@ -260,7 +260,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_chain_set_u8, nft_chain_attr_set_u8); > > int nftnl_chain_set_str(struct nftnl_chain *c, uint16_t attr, const char *str) > { > - return nftnl_chain_set_data(c, attr, str, strlen(str)); > + return nftnl_chain_set_data(c, attr, str, strlen(str) + 1); > } > EXPORT_SYMBOL_ALIAS(nftnl_chain_set_str, nft_chain_attr_set_str); > > diff --git a/src/rule.c b/src/rule.c > index c87fea7..2b23c8e 100644 > --- a/src/rule.c > +++ b/src/rule.c > @@ -198,7 +198,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_rule_set_u64, nft_rule_attr_set_u64); > > int nftnl_rule_set_str(struct nftnl_rule *r, uint16_t attr, const char *str) > { > - return nftnl_rule_set_data(r, attr, str, strlen(str)); > + return nftnl_rule_set_data(r, attr, str, strlen(str) + 1); > } > EXPORT_SYMBOL_ALIAS(nftnl_rule_set_str, nft_rule_attr_set_str); > > diff --git a/src/set_elem.c b/src/set_elem.c > index 00b7327..40b5bfe 100644 > --- a/src/set_elem.c > +++ b/src/set_elem.c > @@ -149,7 +149,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_set_elem_set_u64, nft_set_elem_attr_set_u64); > > int nftnl_set_elem_set_str(struct nftnl_set_elem *s, uint16_t attr, const char *str) > { > - return nftnl_set_elem_set(s, attr, str, strlen(str)); > + return nftnl_set_elem_set(s, attr, str, strlen(str) + 1); > } > EXPORT_SYMBOL_ALIAS(nftnl_set_elem_set_str, nft_set_elem_attr_set_str); > > diff --git a/src/table.c b/src/table.c > index 32d119f..966b923 100644 > --- a/src/table.c > +++ b/src/table.c > @@ -131,7 +131,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_table_set_u8, nft_table_attr_set_u8); > > int nftnl_table_set_str(struct nftnl_table *t, uint16_t attr, const char *str) > { > - return nftnl_table_set_data(t, attr, str, 0); > + return nftnl_table_set_data(t, attr, str, strlen(str) + 1); > } > EXPORT_SYMBOL_ALIAS(nftnl_table_set_str, nft_table_attr_set_str); > > diff --git a/src/trace.c b/src/trace.c > index d8f561d..1a50390 100644 > --- a/src/trace.c > +++ b/src/trace.c > @@ -165,13 +165,13 @@ const void *nftnl_trace_get_data(const struct nftnl_trace *trace, > *data_len = sizeof(uint32_t); > return &trace->type; > case NFTNL_TRACE_CHAIN: > - *data_len = strlen(trace->chain); > + *data_len = strlen(trace->chain) + 1; > return trace->chain; > case NFTNL_TRACE_TABLE: > - *data_len = strlen(trace->table); > + *data_len = strlen(trace->table) + 1; > return trace->table; > case NFTNL_TRACE_JUMP_TARGET: > - *data_len = strlen(trace->jump_target); > + *data_len = strlen(trace->jump_target) + 1; > return trace->jump_target; > case NFTNL_TRACE_TRANSPORT_HEADER: > *data_len = trace->th.len; Are you really sure we need this chunk too? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/07/16 08:54, Pablo Neira Ayuso wrote: > On Fri, Jul 01, 2016 at 06:11:43PM +0200, Carlos Falgueras García wrote: >> These lengths must be one character longer to take account the null >> character > Please, place the change for src/set.c in this patch so I only need to > apply one patch. I will send it now. > Another comment below. > >> Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net> >> --- >> src/chain.c | 2 +- >> src/rule.c | 2 +- >> src/set_elem.c | 2 +- >> src/table.c | 2 +- >> src/trace.c | 6 +++--- >> 5 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/src/chain.c b/src/chain.c >> index bfffbe0..cab64b5 100644 >> --- a/src/chain.c >> +++ b/src/chain.c >> @@ -260,7 +260,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_chain_set_u8, nft_chain_attr_set_u8); >> >> int nftnl_chain_set_str(struct nftnl_chain *c, uint16_t attr, const char *str) >> { >> - return nftnl_chain_set_data(c, attr, str, strlen(str)); >> + return nftnl_chain_set_data(c, attr, str, strlen(str) + 1); >> } >> EXPORT_SYMBOL_ALIAS(nftnl_chain_set_str, nft_chain_attr_set_str); >> >> diff --git a/src/rule.c b/src/rule.c >> index c87fea7..2b23c8e 100644 >> --- a/src/rule.c >> +++ b/src/rule.c >> @@ -198,7 +198,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_rule_set_u64, nft_rule_attr_set_u64); >> >> int nftnl_rule_set_str(struct nftnl_rule *r, uint16_t attr, const char *str) >> { >> - return nftnl_rule_set_data(r, attr, str, strlen(str)); >> + return nftnl_rule_set_data(r, attr, str, strlen(str) + 1); >> } >> EXPORT_SYMBOL_ALIAS(nftnl_rule_set_str, nft_rule_attr_set_str); >> >> diff --git a/src/set_elem.c b/src/set_elem.c >> index 00b7327..40b5bfe 100644 >> --- a/src/set_elem.c >> +++ b/src/set_elem.c >> @@ -149,7 +149,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_set_elem_set_u64, nft_set_elem_attr_set_u64); >> >> int nftnl_set_elem_set_str(struct nftnl_set_elem *s, uint16_t attr, const char *str) >> { >> - return nftnl_set_elem_set(s, attr, str, strlen(str)); >> + return nftnl_set_elem_set(s, attr, str, strlen(str) + 1); >> } >> EXPORT_SYMBOL_ALIAS(nftnl_set_elem_set_str, nft_set_elem_attr_set_str); >> >> diff --git a/src/table.c b/src/table.c >> index 32d119f..966b923 100644 >> --- a/src/table.c >> +++ b/src/table.c >> @@ -131,7 +131,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_table_set_u8, nft_table_attr_set_u8); >> >> int nftnl_table_set_str(struct nftnl_table *t, uint16_t attr, const char *str) >> { >> - return nftnl_table_set_data(t, attr, str, 0); >> + return nftnl_table_set_data(t, attr, str, strlen(str) + 1); >> } >> EXPORT_SYMBOL_ALIAS(nftnl_table_set_str, nft_table_attr_set_str); >> >> diff --git a/src/trace.c b/src/trace.c >> index d8f561d..1a50390 100644 >> --- a/src/trace.c >> +++ b/src/trace.c >> @@ -165,13 +165,13 @@ const void *nftnl_trace_get_data(const struct nftnl_trace *trace, >> *data_len = sizeof(uint32_t); >> return &trace->type; >> case NFTNL_TRACE_CHAIN: >> - *data_len = strlen(trace->chain); >> + *data_len = strlen(trace->chain) + 1; >> return trace->chain; >> case NFTNL_TRACE_TABLE: >> - *data_len = strlen(trace->table); >> + *data_len = strlen(trace->table) + 1; >> return trace->table; >> case NFTNL_TRACE_JUMP_TARGET: >> - *data_len = strlen(trace->jump_target); >> + *data_len = strlen(trace->jump_target) + 1; >> return trace->jump_target; >> case NFTNL_TRACE_TRANSPORT_HEADER: >> *data_len = trace->th.len; > Are you really sure we need this chunk too? Yes, I think the user would expect that 'data_len' means data length instead of data length minus one. When checking other getters I realized that in most cases we keep the parameter 'data_len' unset when the user ask for a string. Maybe we can fix these behavior, I think it make more sense if our getters always return the data length instead of keep an inconstant behavior. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jul 03, 2016 at 12:13:18PM +0200, Carlos Falgueras García wrote: > >>diff --git a/src/trace.c b/src/trace.c > >>index d8f561d..1a50390 100644 > >>--- a/src/trace.c > >>+++ b/src/trace.c > >>@@ -165,13 +165,13 @@ const void *nftnl_trace_get_data(const struct nftnl_trace *trace, > >> *data_len = sizeof(uint32_t); > >> return &trace->type; > >> case NFTNL_TRACE_CHAIN: > >>- *data_len = strlen(trace->chain); > >>+ *data_len = strlen(trace->chain) + 1; > >> return trace->chain; > >> case NFTNL_TRACE_TABLE: > >>- *data_len = strlen(trace->table); > >>+ *data_len = strlen(trace->table) + 1; > >> return trace->table; > >> case NFTNL_TRACE_JUMP_TARGET: > >>- *data_len = strlen(trace->jump_target); > >>+ *data_len = strlen(trace->jump_target) + 1; > >> return trace->jump_target; > >> case NFTNL_TRACE_TRANSPORT_HEADER: > >> *data_len = trace->th.len; > >Are you really sure we need this chunk too? > Yes, I think the user would expect that 'data_len' means data length instead > of data length minus one. > > When checking other getters I realized that in most cases we keep the > parameter 'data_len' unset when the user ask for a string. Maybe we can fix > these behavior, I think it make more sense if our getters always return the > data length instead of keep an inconstant behavior. _get() for strings functions should retain the strlen() semantics. So I'd suggest you just fix the string setters. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 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/src/chain.c b/src/chain.c index bfffbe0..cab64b5 100644 --- a/src/chain.c +++ b/src/chain.c @@ -260,7 +260,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_chain_set_u8, nft_chain_attr_set_u8); int nftnl_chain_set_str(struct nftnl_chain *c, uint16_t attr, const char *str) { - return nftnl_chain_set_data(c, attr, str, strlen(str)); + return nftnl_chain_set_data(c, attr, str, strlen(str) + 1); } EXPORT_SYMBOL_ALIAS(nftnl_chain_set_str, nft_chain_attr_set_str); diff --git a/src/rule.c b/src/rule.c index c87fea7..2b23c8e 100644 --- a/src/rule.c +++ b/src/rule.c @@ -198,7 +198,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_rule_set_u64, nft_rule_attr_set_u64); int nftnl_rule_set_str(struct nftnl_rule *r, uint16_t attr, const char *str) { - return nftnl_rule_set_data(r, attr, str, strlen(str)); + return nftnl_rule_set_data(r, attr, str, strlen(str) + 1); } EXPORT_SYMBOL_ALIAS(nftnl_rule_set_str, nft_rule_attr_set_str); diff --git a/src/set_elem.c b/src/set_elem.c index 00b7327..40b5bfe 100644 --- a/src/set_elem.c +++ b/src/set_elem.c @@ -149,7 +149,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_set_elem_set_u64, nft_set_elem_attr_set_u64); int nftnl_set_elem_set_str(struct nftnl_set_elem *s, uint16_t attr, const char *str) { - return nftnl_set_elem_set(s, attr, str, strlen(str)); + return nftnl_set_elem_set(s, attr, str, strlen(str) + 1); } EXPORT_SYMBOL_ALIAS(nftnl_set_elem_set_str, nft_set_elem_attr_set_str); diff --git a/src/table.c b/src/table.c index 32d119f..966b923 100644 --- a/src/table.c +++ b/src/table.c @@ -131,7 +131,7 @@ EXPORT_SYMBOL_ALIAS(nftnl_table_set_u8, nft_table_attr_set_u8); int nftnl_table_set_str(struct nftnl_table *t, uint16_t attr, const char *str) { - return nftnl_table_set_data(t, attr, str, 0); + return nftnl_table_set_data(t, attr, str, strlen(str) + 1); } EXPORT_SYMBOL_ALIAS(nftnl_table_set_str, nft_table_attr_set_str); diff --git a/src/trace.c b/src/trace.c index d8f561d..1a50390 100644 --- a/src/trace.c +++ b/src/trace.c @@ -165,13 +165,13 @@ const void *nftnl_trace_get_data(const struct nftnl_trace *trace, *data_len = sizeof(uint32_t); return &trace->type; case NFTNL_TRACE_CHAIN: - *data_len = strlen(trace->chain); + *data_len = strlen(trace->chain) + 1; return trace->chain; case NFTNL_TRACE_TABLE: - *data_len = strlen(trace->table); + *data_len = strlen(trace->table) + 1; return trace->table; case NFTNL_TRACE_JUMP_TARGET: - *data_len = strlen(trace->jump_target); + *data_len = strlen(trace->jump_target) + 1; return trace->jump_target; case NFTNL_TRACE_TRANSPORT_HEADER: *data_len = trace->th.len;
These lengths must be one character longer to take account the null character Signed-off-by: Carlos Falgueras García <carlosfg@riseup.net> --- src/chain.c | 2 +- src/rule.c | 2 +- src/set_elem.c | 2 +- src/table.c | 2 +- src/trace.c | 6 +++--- 5 files changed, 7 insertions(+), 7 deletions(-)