Web lists-archives.com

Re: [PATCH v2] xgethostname: handle long hostnames




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

> > -			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