[PATCH nft 1/3] parser: don't mark "string" as const

[PATCH nft 1/3] parser: don't mark "string" as const

[Thomas Haller]

The "string" field is allocated, and the bison actions are expected to
take/free them. It's not const, and it should not be freed with free_const().

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/parser_bison.y | 148 ++++++++++++++++++++++-----------------------
 1 file changed, 74 insertions(+), 74 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 1e8169c44f62..ca0851c915d2 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -148,19 +148,19 @@ static bool already_set(const void *attr, const struct location *loc,
 
 static struct expr *ifname_expr_alloc(const struct location *location,
 				      struct list_head *queue,
-				      const char *name)
+				      char *name)
 {
 	unsigned int length = strlen(name);
 	struct expr *expr;
 
 	if (length == 0) {
-		free_const(name);
+		free(name);
 		erec_queue(error(location, "empty interface name"), queue);
 		return NULL;
 	}
 
 	if (length >= IFNAMSIZ) {
-		free_const(name);
+		free(name);
 		erec_queue(error(location, "interface name too long"), queue);
 		return NULL;
 	}
@@ -168,7 +168,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
 	expr = constant_expr_alloc(location, &ifname_type, BYTEORDER_HOST_ENDIAN,
 				   length * BITS_PER_BYTE, name);
 
-	free_const(name);
+	free(name);
 
 	return expr;
 }
@@ -207,7 +207,7 @@ int nft_lex(void *, void *, void *);
 	uint64_t		val;
 	uint32_t		val32;
 	uint8_t			val8;
-	const char *		string;
+	char *			string;
 
 	struct list_head	*list;
 	struct cmd		*cmd;
@@ -358,7 +358,7 @@ int nft_lex(void *, void *, void *);
 %token <string> STRING		"string"
 %token <string> QUOTED_STRING	"quoted string"
 %token <string> ASTERISK_STRING	"string with a trailing asterisk"
-%destructor { free_const($$); }	STRING QUOTED_STRING ASTERISK_STRING
+%destructor { free($$); }	STRING QUOTED_STRING ASTERISK_STRING
 
 %token LL_HDR			"ll"
 %token NETWORK_HDR		"nh"
@@ -674,7 +674,7 @@ int nft_lex(void *, void *, void *);
 %type <limit_rate>		limit_rate_bytes
 
 %type <string>			identifier type_identifier string comment_spec
-%destructor { free_const($$); }	identifier type_identifier string comment_spec
+%destructor { free($$); }	identifier type_identifier string comment_spec
 
 %type <val>			time_spec time_spec_or_num_s quota_used
 
@@ -709,7 +709,7 @@ int nft_lex(void *, void *, void *);
 %type <val32>			int_num	chain_policy
 %type <prio_spec>		extended_prio_spec prio_spec
 %type <string>			extended_prio_name quota_unit	basehook_device_name
-%destructor { free_const($$); }	extended_prio_name quota_unit	basehook_device_name
+%destructor { free($$); }	extended_prio_name quota_unit	basehook_device_name
 
 %type <expr>			dev_spec
 %destructor { free($$); }	dev_spec
@@ -928,7 +928,7 @@ int nft_lex(void *, void *, void *);
 
 %type <val>			markup_format
 %type <string>			monitor_event
-%destructor { free_const($$); }	monitor_event
+%destructor { free($$); }	monitor_event
 %type <val>			monitor_object	monitor_format
 
 %type <val>			synproxy_ts	synproxy_sack
@@ -1053,10 +1053,10 @@ close_scope_xt		: { scanner_pop_start_cond(nft->scanner, PARSER_SC_XT); }
 common_block		:	INCLUDE		QUOTED_STRING	stmt_separator
 			{
 				if (scanner_include_file(nft, scanner, $2, &@$) < 0) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
-				free_const($2);
+				free($2);
 			}
 			|	DEFINE		identifier	'='	initializer_expr	stmt_separator
 			{
@@ -1066,19 +1066,19 @@ common_block		:	INCLUDE		QUOTED_STRING	stmt_separator
 					erec_queue(error(&@2, "redefinition of symbol '%s'", $2),
 						   state->msgs);
 					expr_free($4);
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 
 				symbol_bind(scope, $2, $4);
-				free_const($2);
+				free($2);
 			}
 			|	REDEFINE	identifier	'='	initializer_expr	stmt_separator
 			{
 				struct scope *scope = current_scope(state);
 
 				symbol_bind(scope, $2, $4);
-				free_const($2);
+				free($2);
 			}
 			|	UNDEFINE	identifier	stmt_separator
 			{
@@ -1087,10 +1087,10 @@ common_block		:	INCLUDE		QUOTED_STRING	stmt_separator
 				if (symbol_unbind(scope, $2) < 0) {
 					erec_queue(error(&@2, "undefined symbol '%s'", $2),
 						   state->msgs);
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
-				free_const($2);
+				free($2);
 			}
 			|	error		stmt_separator
 			{
@@ -1879,21 +1879,21 @@ table_options		:	FLAGS		STRING
 			{
 				if (strcmp($2, "dormant") == 0) {
 					$<table>0->flags |= TABLE_F_DORMANT;
-					free_const($2);
+					free($2);
 				} else if (strcmp($2, "owner") == 0) {
 					$<table>0->flags |= TABLE_F_OWNER;
-					free_const($2);
+					free($2);
 				} else {
 					erec_queue(error(&@2, "unknown table option %s", $2),
 						   state->msgs);
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 			}
 			|	comment_spec
 			{
 				if (already_set($<table>0->comment, &@$, state)) {
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
 				$<table>0->comment = $1;
@@ -2064,7 +2064,7 @@ chain_block		:	/* empty */	{ $$ = $<chain>-1; }
 			|	chain_block	comment_spec	stmt_separator
 			{
 				if (already_set($1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$1->comment = $2;
@@ -2190,7 +2190,7 @@ set_block		:	/* empty */	{ $$ = $<set>-1; }
 			|	set_block	comment_spec	stmt_separator
 			{
 				if (already_set($1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$1->comment = $2;
@@ -2307,7 +2307,7 @@ map_block		:	/* empty */	{ $$ = $<set>-1; }
 			|	map_block	comment_spec	stmt_separator
 			{
 				if (already_set($1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$1->comment = $2;
@@ -2346,10 +2346,10 @@ flowtable_block		:	/* empty */	{ $$ = $<flowtable>-1; }
 				if ($$->hook.name == NULL) {
 					erec_queue(error(&@3, "unknown chain hook"),
 						   state->msgs);
-					free_const($3);
+					free($3);
 					YYERROR;
 				}
-				free_const($3);
+				free($3);
 
 				$$->priority = $4;
 			}
@@ -2423,12 +2423,12 @@ data_type_atom_expr	:	type_identifier
 				if (dtype == NULL) {
 					erec_queue(error(&@1, "unknown datatype %s", $1),
 						   state->msgs);
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
 				$$ = constant_expr_alloc(&@1, dtype, dtype->byteorder,
 							 dtype->size, NULL);
-				free_const($1);
+				free($1);
 			}
 			|	TIME
 			{
@@ -2465,7 +2465,7 @@ counter_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|	counter_block	  comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2482,7 +2482,7 @@ quota_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|	quota_block	comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2499,7 +2499,7 @@ ct_helper_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|       ct_helper_block     comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2520,7 +2520,7 @@ ct_timeout_block	:	/*empty */
 			|       ct_timeout_block     comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2537,7 +2537,7 @@ ct_expect_block		:	/*empty */	{ $$ = $<obj>-1; }
 			|       ct_expect_block     comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2554,7 +2554,7 @@ limit_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|       limit_block     comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2571,7 +2571,7 @@ secmark_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|       secmark_block     comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2588,7 +2588,7 @@ synproxy_block		:	/* empty */	{ $$ = $<obj>-1; }
 			|       synproxy_block     comment_spec
 			{
 				if (already_set($<obj>1->comment, &@2, state)) {
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$<obj>1->comment = $2;
@@ -2609,12 +2609,12 @@ hook_spec		:	TYPE		close_scope_type	STRING		HOOK		STRING		dev_spec	prio_spec
 				if (chain_type == NULL) {
 					erec_queue(error(&@3, "unknown chain type"),
 						   state->msgs);
-					free_const($3);
+					free($3);
 					YYERROR;
 				}
 				$<chain>0->type.loc = @3;
 				$<chain>0->type.str = xstrdup(chain_type);
-				free_const($3);
+				free($3);
 
 				$<chain>0->loc = @$;
 				$<chain>0->hook.loc = @5;
@@ -2622,10 +2622,10 @@ hook_spec		:	TYPE		close_scope_type	STRING		HOOK		STRING		dev_spec	prio_spec
 				if ($<chain>0->hook.name == NULL) {
 					erec_queue(error(&@5, "unknown chain hook"),
 						   state->msgs);
-					free_const($5);
+					free($5);
 					YYERROR;
 				}
-				free_const($5);
+				free($5);
 
 				$<chain>0->dev_expr	= $6;
 				$<chain>0->priority	= $7;
@@ -2672,7 +2672,7 @@ extended_prio_spec	:	int_num
 								BYTEORDER_HOST_ENDIAN,
 								strlen($1) * BITS_PER_BYTE,
 								$1);
-				free_const($1);
+				free($1);
 				$$ = spec;
 			}
 			|	extended_prio_name PLUS NUM
@@ -2685,7 +2685,7 @@ extended_prio_spec	:	int_num
 								BYTEORDER_HOST_ENDIAN,
 								strlen(str) * BITS_PER_BYTE,
 								str);
-				free_const($1);
+				free($1);
 				$$ = spec;
 			}
 			|	extended_prio_name DASH NUM
@@ -2698,7 +2698,7 @@ extended_prio_spec	:	int_num
 								BYTEORDER_HOST_ENDIAN,
 								strlen(str) * BITS_PER_BYTE,
 								str);
-				free_const($1);
+				free($1);
 				$$ = spec;
 			}
 			;
@@ -2783,7 +2783,7 @@ time_spec		:	STRING
 				uint64_t res;
 
 				erec = time_parse(&@1, $1, &res);
-				free_const($1);
+				free($1);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -2984,7 +2984,7 @@ comment_spec		:	COMMENT		string
 					erec_queue(error(&@2, "comment too long, %d characters maximum allowed",
 							 NFTNL_UDATA_COMMENT_MAXLEN),
 						   state->msgs);
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 				$$ = $2;
@@ -3085,8 +3085,8 @@ stmt			:	verdict_stmt
 xt_stmt			:	XT	STRING	string
 			{
 				$$ = NULL;
-				free_const($2);
-				free_const($3);
+				free($2);
+				free($3);
 				erec_queue(error(&@$, "unsupported xtables compat expression, use iptables-nft with this ruleset"),
 					   state->msgs);
 				YYERROR;
@@ -3244,7 +3244,7 @@ log_arg			:	PREFIX			string
 					expr = constant_expr_alloc(&@$, &string_type,
 								   BYTEORDER_HOST_ENDIAN,
 								   (strlen($2) + 1) * BITS_PER_BYTE, $2);
-					free_const($2);
+					free($2);
 					$<stmt>0->log.prefix = expr;
 					$<stmt>0->log.flags |= STMT_LOG_PREFIX;
 					break;
@@ -3318,7 +3318,7 @@ log_arg			:	PREFIX			string
 									   state->msgs);
 							}
 							expr_free(expr);
-							free_const($2);
+							free($2);
 							YYERROR;
 						}
 						item = variable_expr_alloc(&@$, scope, sym);
@@ -3348,7 +3348,7 @@ log_arg			:	PREFIX			string
 					}
 				}
 
-				free_const($2);
+				free($2);
 				$<stmt>0->log.prefix	 = expr;
 				$<stmt>0->log.flags 	|= STMT_LOG_PREFIX;
 			}
@@ -3401,10 +3401,10 @@ level_type		:	string
 				else {
 					erec_queue(error(&@1, "invalid log level"),
 						   state->msgs);
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
-				free_const($1);
+				free($1);
 			}
 			;
 
@@ -3494,7 +3494,7 @@ quota_used		:	/* empty */	{ $$ = 0; }
 				uint64_t rate;
 
 				erec = data_unit_parse(&@$, $3, &rate);
-				free_const($3);
+				free($3);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -3509,7 +3509,7 @@ quota_stmt		:	QUOTA	quota_mode NUM quota_unit quota_used	close_scope_quota
 				uint64_t rate;
 
 				erec = data_unit_parse(&@$, $4, &rate);
-				free_const($4);
+				free($4);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -3553,7 +3553,7 @@ limit_rate_bytes	:	NUM     STRING
 				uint64_t rate, unit;
 
 				erec = rate_parse(&@$, $2, &rate, &unit);
-				free_const($2);
+				free($2);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -3575,7 +3575,7 @@ limit_bytes		:	NUM	BYTES		{ $$ = $1; }
 				uint64_t rate;
 
 				erec = data_unit_parse(&@$, $2, &rate);
-				free_const($2);
+				free($2);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -3604,7 +3604,7 @@ reject_with_expr	:	STRING
 			{
 				$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
 						       current_scope(state), $1);
-				free_const($1);
+				free($1);
 			}
 			|	integer_expr	{ $$ = $1; }
 			;
@@ -4268,12 +4268,12 @@ variable_expr		:	'$'	identifier
 						erec_queue(error(&@2, "unknown identifier '%s'", $2),
 							   state->msgs);
 					}
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
 
 				$$ = variable_expr_alloc(&@$, scope, sym);
-				free_const($2);
+				free($2);
 			}
 			;
 
@@ -4283,7 +4283,7 @@ symbol_expr		:	variable_expr
 				$$ = symbol_expr_alloc(&@$, SYMBOL_VALUE,
 						       current_scope(state),
 						       $1);
-				free_const($1);
+				free($1);
 			}
 			;
 
@@ -4296,7 +4296,7 @@ set_ref_symbol_expr	:	AT	identifier	close_scope_at
 				$$ = symbol_expr_alloc(&@$, SYMBOL_SET,
 						       current_scope(state),
 						       $2);
-				free_const($2);
+				free($2);
 			}
 			;
 
@@ -4393,10 +4393,10 @@ osf_ttl			:	/* empty */
 				else {
 					erec_queue(error(&@2, "invalid ttl option"),
 						   state->msgs);
-					free_const($2);
+					free($2);
 					YYERROR;
 				}
-				free_const($2);
+				free($2);
 			}
 			;
 
@@ -4566,7 +4566,7 @@ set_elem_option		:	TIMEOUT			time_spec
 			|	comment_spec
 			{
 				if (already_set($<expr>0->comment, &@1, state)) {
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
 				$<expr>0->comment = $1;
@@ -4648,7 +4648,7 @@ set_elem_stmt		:	COUNTER	close_scope_counter
 				uint64_t rate;
 
 				erec = data_unit_parse(&@$, $4, &rate);
-				free_const($4);
+				free($4);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -4681,7 +4681,7 @@ set_elem_expr_option	:	TIMEOUT			time_spec
 			|	comment_spec
 			{
 				if (already_set($<expr>0->comment, &@1, state)) {
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
 				$<expr>0->comment = $1;
@@ -4733,7 +4733,7 @@ quota_config		:	quota_mode NUM quota_unit quota_used
 				uint64_t rate;
 
 				erec = data_unit_parse(&@$, $3, &rate);
-				free_const($3);
+				free($3);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -4762,10 +4762,10 @@ secmark_config		:	string
 				ret = snprintf(secmark->ctx, sizeof(secmark->ctx), "%s", $1);
 				if (ret <= 0 || ret >= (int)sizeof(secmark->ctx)) {
 					erec_queue(error(&@1, "invalid context '%s', max length is %u\n", $1, (int)sizeof(secmark->ctx)), state->msgs);
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
-				free_const($1);
+				free($1);
 			}
 			;
 
@@ -4802,7 +4802,7 @@ ct_helper_config		:	TYPE	QUOTED_STRING	PROTOCOL	ct_l4protoname	stmt_separator	cl
 					erec_queue(error(&@2, "invalid name '%s', max length is %u\n", $2, (int)sizeof(ct->name)), state->msgs);
 					YYERROR;
 				}
-				free_const($2);
+				free($2);
 
 				ct->l4proto = $4;
 			}
@@ -5197,7 +5197,7 @@ chain_expr		:	variable_expr
 							 BYTEORDER_HOST_ENDIAN,
 							 strlen($1) * BITS_PER_BYTE,
 							 $1);
-				free_const($1);
+				free($1);
 			}
 			;
 
@@ -5215,7 +5215,7 @@ meta_expr		:	META	meta_key	close_scope_meta
 				unsigned int key;
 
 				erec = meta_key_parse(&@$, $2, &key);
-				free_const($2);
+				free($2);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -5292,7 +5292,7 @@ meta_stmt		:	META	meta_key	SET	stmt_expr	close_scope_meta
 				unsigned int key;
 
 				erec = meta_key_parse(&@$, $2, &key);
-				free_const($2);
+				free($2);
 				if (erec != NULL) {
 					erec_queue(erec, state->msgs);
 					YYERROR;
@@ -5603,10 +5603,10 @@ payload_base_spec	:	LL_HDR		{ $$ = PROTO_BASE_LL_HDR; }
 					$$ = PROTO_BASE_INNER_HDR;
 				} else {
 					erec_queue(error(&@1, "unknown raw payload base"), state->msgs);
-					free_const($1);
+					free($1);
 					YYERROR;
 				}
-				free_const($1);
+				free($1);
 			}
 			;
 
-- 
2.41.0

[PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy()

[Thomas Haller]

It's not a const pointer, as the destroy() function clearly
modifies/free is. Drop the const from the argument of
input_descriptor_destroy().

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/scanner.l | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/scanner.l b/src/scanner.l
index 00a09485d420..31284d7358fa 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -1258,11 +1258,11 @@ void *scanner_init(struct parser_state *state)
 	return scanner;
 }
 
-static void input_descriptor_destroy(const struct input_descriptor *indesc)
+static void input_descriptor_destroy(struct input_descriptor *indesc)
 {
 	if (indesc->name)
 		free_const(indesc->name);
-	free_const(indesc);
+	free(indesc);
 }
 
 static void input_descriptor_list_destroy(struct parser_state *state)
-- 
2.41.0

Re: [PATCH nft 2/3] parser: remove "const" from argument of input_descriptor_destroy()

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> It's not a const pointer, as the destroy() function clearly
> modifies/free is. Drop the const from the argument of
> input_descriptor_destroy().
> 
> Signed-off-by: Thomas Haller <thaller@redhat.com>
> ---
>  src/scanner.l | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/scanner.l b/src/scanner.l
> index 00a09485d420..31284d7358fa 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -1258,11 +1258,11 @@ void *scanner_init(struct parser_state *state)
>  	return scanner;
>  }
>  
> -static void input_descriptor_destroy(const struct input_descriptor *indesc)
> +static void input_descriptor_destroy(struct input_descriptor *indesc)
>  {
>  	if (indesc->name)
>  		free_const(indesc->name);
> -	free_const(indesc);
> +	free(indesc);

I don't agree, this is fine as-is.

[PATCH nft 3/3] parser: use size_t type for strlen() results

[Thomas Haller]

strlen() has a "size_t" as result. Use the correct type.

Signed-off-by: Thomas Haller <thaller@redhat.com>
---
 src/parser_bison.y | 2 +-
 src/scanner.l      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index ca0851c915d2..12031c831353 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -150,7 +150,7 @@ static struct expr *ifname_expr_alloc(const struct location *location,
 				      struct list_head *queue,
 				      char *name)
 {
-	unsigned int length = strlen(name);
+	size_t length = strlen(name);
 	struct expr *expr;
 
 	if (length == 0) {
diff --git a/src/scanner.l b/src/scanner.l
index 31284d7358fa..aba8ef1c6b54 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -1117,7 +1117,7 @@ static int include_glob(struct nft_ctx *nft, void *scanner, const char *pattern,
 	ret = glob(pattern, flags, NULL, &glob_data);
 	if (ret == 0) {
 		char *path;
-		int len;
+		size_t len;
 
 		/* reverse alphabetical order due to stack */
 		for (i = glob_data.gl_pathc; i > 0; i--) {
-- 
2.41.0

Re: [PATCH nft 3/3] parser: use size_t type for strlen() results

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> strlen() has a "size_t" as result. Use the correct type.

I've applied this one, but seriously, seems quite a bit of
bikeshedding.

Re: [PATCH nft 1/3] parser: don't mark "string" as const

[Pablo Neira Ayuso]

On Thu, Nov 09, 2023 at 07:59:47PM +0100, Thomas Haller wrote:
> The "string" field is allocated, and the bison actions are expected to
> take/free them. It's not const, and it should not be freed with free_const().

This ifname_expr_alloc() function does not modify the 'name' argument,
this is really const.

Re: [PATCH nft 1/3] parser: don't mark "string" as const

[Thomas Haller]

On Thu, 2023-11-09 at 20:22 +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 09, 2023 at 07:59:47PM +0100, Thomas Haller wrote:
> > The "string" field is allocated, and the bison actions are expected
> > to
> > take/free them. It's not const, and it should not be freed with
> > free_const().
> 
> This ifname_expr_alloc() function does not modify the 'name'
> argument,
> this is really const.

ifname_expr_alloc() calls `free(name)`, destroying the thing that is
pointed at. That's a modification.

The code required to cast away the constness, which also indicates that
it is not actually const.


Thomas

Re: [PATCH nft 1/3] parser: don't mark "string" as const

[Florian Westphal]

Thomas Haller <thaller@redhat.com> wrote:
> ifname_expr_alloc() calls `free(name)`, destroying the thing that is
> pointed at. That's a modification.
> 
> The code required to cast away the constness, which also indicates that
> it is not actually const.

It it.  I hate that free() isn't const void *.

I prefer to have const used as much as humanly possible.

Re: [PATCH nft 1/3] parser: don't mark "string" as const

[Thomas Haller]

On Fri, 2023-11-10 at 00:10 +0100, Florian Westphal wrote:
> Thomas Haller <thaller@redhat.com> wrote:
> > ifname_expr_alloc() calls `free(name)`, destroying the thing that
> > is
> > pointed at. That's a modification.
> > 
> > The code required to cast away the constness, which also indicates
> > that
> > it is not actually const.
> 
> It it.  I hate that free() isn't const void *.

That would work too. But C choose instead to disallow that.

Note that in nftables, ~75% of the free() operate on non-const just
fine. And the compiler is helpful confirming that.

> 
> I prefer to have const used as much as humanly possible.
> 

Definitely. Especially for long-lived data.


The "string" in question is a short-lived data allocated by "scanner.l"
and handed over to "parser_bison.y".

See:

»·······»·······»·······|»······rule_alloc»·····comment_spec
»·······»·······»·······{
»·······»·······»·······»·······$$->cOmment = $2;
»·······»·······»·······}

Here no free() is involved, but ownership is passed on without
strdup(). If "string" is const, this is another ugliness where you pass
on a string that you seemingly don't own.


There are places where it's ugly and const-correctness breaks down
(during destruction/ref/unref). See free_const() and
datatype_get()/datatype_free(). But those places should be at a minimum
by choosing const (and free_const()) when it makes sense.



Thomas