Re: [PATCH v2] xgethostname: handle long hostnames

From: Jeff King <peff@peff.net>
Date: Apr 18, 2017, 16:17 UTC
Message-ID: <20170418161734.pa665rqwdtbnsj7f@sigill.intra.peff.net>
In-Reply-To: <281d0843-d48a-b7ab-737b-b9528689d44e@web.de>

On Tue, Apr 18, 2017 at 06:07:43PM +0200, René Scharfe wrote:

Show 12 quoted lines

> > -			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> > +			fscanf(fp, scan_fmt, &pid, locking_host) == 2 &&
> >   			/* be gentle to concurrent "gc" on remote hosts */
> >   			(strcmp(locking_host, my_host) || !kill(pid, 0) || errno == EPERM);
> >   		if (fp != NULL)
> > 
> 
> How important is it to scan the whole file in one call?  We could split
> it up like this and use a strbuf to handle host names of any length.  We
> need to be permissive here to allow machines with different values for
> HOST_NAME_MAX to work with the same file on a network file system, so
> this would have to be the first patch, right?

I doubt that doing it in one call matters. It's not like stdio promises us any atomicity in the first place.

> -			fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2 &&
> +			fscanf(fp, "%"SCNuMAX" ", &pid) == 1 &&
> +			!strbuf_getwholeline(&locking_host, fp, '\0') &&

I don't think there is anything wrong with using fscanf here, but it has enough pitfalls in general that I don't really like its use as a parser (and the general lack of it in Git's code base seems to agree).

I wonder if this should just read a line (or the whole file) into a strbuf and parse it there. That would better match our usual style, I think.

I can live with it either way.

> NB: That && cascade has enough meat for a whole function.

Yeah.

-Peff