Re: [iptables PATCH 6/8] xtables-restore: Drop pointless newargc reset

[Phil Sutter <phil@nwl.cc>]

Hi,

On Fri, Oct 18, 2019 at 10:30:56AM +0200, Pablo Neira Ayuso wrote:
> On Fri, Oct 18, 2019 at 12:48:34AM +0200, Phil Sutter wrote:
> > This was overlooked when merging argv-related code: newargc is
> > initialized at declaration and reset in free_argv() again.
> > 
> > Fixes: a2ed880a19d08 ("xshared: Consolidate argv construction routines")
> > Signed-off-by: Phil Sutter <phil@nwl.cc>
> > ---
> >  iptables/xtables-restore.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/iptables/xtables-restore.c b/iptables/xtables-restore.c
> > index df8844208c273..bb6ee78933f7a 100644
> > --- a/iptables/xtables-restore.c
> > +++ b/iptables/xtables-restore.c
> > @@ -232,9 +232,6 @@ void xtables_restore_parse(struct nft_handle *h,
> >  			char *bcnt = NULL;
> >  			char *parsestart = buffer;
> >  
> > -			/* reset the newargv */
> > -			newargc = 0;
> 
> Are you sure this is correct? This resets the variable for each table
> this is entering.

In fact, the removed line resets newargc before parsing each rule line.
But since newargc is initially zero and after each call to do_command a
call to free_argv() happens which resets newargc again, we're really
save here.

> BTW, newargv, newargc are defined as globals which is very hard to
> follow when reading this code. Probably place them in a structure
> definition and pass them to functions to make easier to follow track
> of this code?

Good point, I'll do that.

> That code would qualify for placing it under
> iptables/xtables-restore.c since it is common for the xml and the
> native parser as I suggested before.

These global variables and related functions currently reside in
xshared.c which is the right spot. :)

Thanks, Phil