Re: [PATCH 3/3] fwd, conf: Probe host's ephemeral ports

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 29 Aug 2024 11:29:30 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Aug 28, 2024 at 12:22:18PM +0200, Laurent Vivier wrote:
> > On 28/08/2024 07:56, David Gibson wrote:  
> > >
> > > [...]
> > >
> > > +	lineread_init(&lr, fd);
> > > +	len = lineread_get(&lr, &line);
> > > +	if (len < 0)
> > > +		goto parse_err;
> > > +
> > > +	tab = strchr(line, '\t');
> > > +	if (!tab)
> > > +		goto parse_err;
> > > +	*tab = '\0';
> > > +
> > > +	errno = 0;
> > > +	min = strtol(line, &end, 10);
> > > +	if (*end || errno)
> > > +		goto parse_err;
> > > +
> > > +	errno = 0;
> > > +	max = strtol(tab + 1, &end, 10);
> > > +	if (*end || errno)
> > > +		goto parse_err;  
> > 
> > As /proc files are well formated, why don't you use fscanf()?
> > Something like:
> > 
> >         FILE *f;
> > 
> >         f = fopen(PORT_RANGE_SYSCTL, "r");
> > 	if (f == NULL) {
> > 		warn("Unable to parse %s", PORT_RANGE_SYSCTL);
> > 		return;
> > 	}
> >         ret = fscanf(f, "%d %d", &min, &max);
> >         fclose(f);
> >         if (ret != 2)
> >                 goto parse_error;  
> 
> Hm, maybe.  I never feel like I know exactly what the parse rules for
> scanf() are, so I tend to avoid it.  Stefano, any thoughts?

The issue with fopen() and ffscanf() is that they need dynamic
allocation, see also 32d07f5e59f2 ("passt, pasta: Completely avoid
dynamic memory allocation").

Still, here, we could use lineread_get() and sscanf() as we do in fwd.c,
procfs_scan_listen(), but I don't really have a preference. If you find
that scanf() is confusing for you, perhaps it would be better to use
something simpler as you did. On the other hand it would be 2 lines of
code instead of (somewhat bug-prone) 14 lines.

I'm not sure, I'd say whatever you feel most comfortable with...

-- 
Stefano