Re: [PATCH v2] xgethostname: handle long hostnames
- Date: Tue, 18 Apr 2017 12:17:34 -0400
- From: Jeff King <peff@xxxxxxxx>
- Subject: 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
I can live with it either way.
> NB: That && cascade has enough meat for a whole function.