Re: [PATCH v2] xgethostname: handle long hostnames
David Turner writes: > If the writer has the smaller HOST_NAME_MAX, this will work fine. If the > reader > has the smaller HOST_NAME_MAX, and the writer's actual value is too long, > then there's no way the strcmp would succeed anyway. So I don't think we need > to worry about it. Hmph, I have to agree with that reasoning, only because the value we read into locking_host[] is not used for error reporting at all. I would have insisted to read what is on the filesystem anyway if that were not the case. Thanks.
Re: [PATCH v2] xgethostname: handle long hostnames
René Scharfe writes:
> 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?
Absolutely. FWIW, I agree with Peff that we do not need to use
fscanf here; just reading a line into strbuf and picking pieces
would be sufficient.
> NB: That && cascade has enough meat for a whole function.
True, too.
>
> René
>
> ---
> builtin/gc.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index 1fca84c19d..d5e880028e 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -251,10 +251,9 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> fd = hold_lock_file_for_update(&lock, pidfile_path,
> LOCK_DIE_ON_ERROR);
> if (!force) {
> - static char locking_host[128];
> + static struct strbuf locking_host = STRBUF_INIT;
> int should_exit;
> fp = fopen(pidfile_path, "r");
> - memset(locking_host, 0, sizeof(locking_host));
> should_exit =
> fp != NULL &&
> !fstat(fileno(fp), &st) &&
> @@ -268,9 +267,10 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
>* running.
>*/
> time(NULL) - st.st_mtime <= 12 * 3600 &&
> - fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2
> &&
> + fscanf(fp, "%"SCNuMAX" ", &pid) == 1 &&
> + !strbuf_getwholeline(&locking_host, fp, '\0') &&
> /* be gentle to concurrent "gc" on remote hosts */
> - (strcmp(locking_host, my_host) || !kill(pid, 0) ||
> errno == EPERM);
> + (strcmp(locking_host.buf, my_host) || !kill(pid, 0) ||
> errno == EPERM);
> if (fp != NULL)
> fclose(fp);
> if (should_exit) {
> @@ -278,7 +278,7 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> rollback_lock_file(&lock);
> *ret_pid = pid;
> free(pidfile_path);
> - return locking_host;
> + return locking_host.buf;
> }
> }
Re: [PATCH v2] xgethostname: handle long hostnames
Jeff King writes: > 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. Yeah, I think it would be a good change.
RE: [PATCH v2] xgethostname: handle long hostnames
> -Original Message-
> From: René Scharfe [mailto:l@web.de]
> Sent: Tuesday, April 18, 2017 12:08 PM
> To: Junio C Hamano ; David Turner
...
> >> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are comparing
> >> it with locking_host, so perhaps we'd need to take this version to
> >> size locking_host to also HOST_NAME_MAX + 1, and then scan with %255c
> >> (but then shouldn't we scan with %256c instead? I am not sure where
> >> these +1 comes from).
> >
> > That is, something along this line...
> >
> > builtin/gc.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/gc.c b/builtin/gc.c index be75508292..4f85610d87
> > 100644
> > --- a/builtin/gc.c
> > +++ b/builtin/gc.c
> > @@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> >LOCK_DIE_ON_ERROR);
> > if (!force) {
> > static char locking_host[HOST_NAME_MAX + 1];
> > + static char *scan_fmt;
> > int should_exit;
> > +
> > + if (!scan_fmt)
> > + scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX,
> HOST_NAME_MAX);
> > fp = fopen(pidfile_path, "r");
> > memset(locking_host, 0, sizeof(locking_host));
> > should_exit =
> > @@ -256,7 +260,7 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> > * running.
> > */
> > time(NULL) - st.st_mtime <= 12 * 3600 &&
> > - 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?
If the writer has the smaller HOST_NAME_MAX, this will work fine. If the reader
has the smaller HOST_NAME_MAX, and the writer's actual value is too long,
then there's no way the strcmp would succeed anyway. So I don't think we need
to worry about it.
> NB: That && cascade has enough meat for a whole function.
+1
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
Re: [PATCH v2] xgethostname: handle long hostnames
Am 18.04.2017 um 03:41 schrieb Junio C Hamano:
> Junio C Hamano writes:
>
>> David Turner writes:
>>
>>> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t*
>>> ret_pid)
>>> ...
>>> if (!force) {
>>> - static char locking_host[128];
>>> + static char locking_host[HOST_NAME_MAX + 1];
>>> int should_exit;
>>> fp = fopen(pidfile_path, "r");
>>> memset(locking_host, 0, sizeof(locking_host));
>>
>> I compared the result of applying this v2 directly on top of master
>> and applying René's "Use HOST_NAME_MAX"and then applying your v1.
>> This hunk is the only difference.
>>
>> As this locking_host is used like so in the later part of the code:
>>
>> time(NULL) - st.st_mtime <= 12 * 3600 &&
>> fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2
>> &&
>> /* be gentle to concurrent "gc" on remote hosts */
>> (strcmp(locking_host, my_host) || !kill(pid, 0) ||
>> errno == EPERM);
>>
>> I suspect that turning it to HOST_NAME_MAX + 1 without tweaking
>> the format "%127c" gives us an inconsistent resulting code.
Oh, missed that. Thanks for catching it!
>> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are
>> comparing it with locking_host, so perhaps we'd need to take this
>> version to size locking_host to also HOST_NAME_MAX + 1, and then
>> scan with %255c (but then shouldn't we scan with %256c instead? I
>> am not sure where these +1 comes from).
>
> That is, something along this line...
>
> builtin/gc.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/gc.c b/builtin/gc.c
> index be75508292..4f85610d87 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> LOCK_DIE_ON_ERROR);
> if (!force) {
> static char locking_host[HOST_NAME_MAX + 1];
> + static char *scan_fmt;
> int should_exit;
> +
> + if (!scan_fmt)
> + scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX,
> HOST_NAME_MAX);
> fp = fopen(pidfile_path, "r");
> memset(locking_host, 0, sizeof(locking_host));
> should_exit =
> @@ -256,7 +260,7 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
>* running.
>*/
> time(NULL) - st.st_mtime <= 12 * 3600 &&
> - 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?
NB: That && cascade has enough meat for a whole function.
René
---
builtin/gc.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index 1fca84c19d..d5e880028e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -251,10 +251,9 @@ static const char *lock_repo_for_gc(int force, pid_t*
ret_pid)
fd = hold_lock_file_for_update(&lock, pidfile_path,
LOCK_DIE_ON_ERROR);
if (!force) {
- static char locking_host[128];
+ static struct strbuf locking_host = STRBUF_INIT;
int should_exit;
fp = fopen(pidfile_path, "r");
- memset(locking_host, 0, sizeof(locking_host));
should_exit =
fp != NULL &&
!fstat(fileno(fp), &st) &&
@@ -268,9 +267,10 @@ static const char *lock_repo_for_gc(int force, pid_t*
ret_pid)
* running.
*/
time(NULL) - st.st_mtime <= 12 * 3600 &&
- fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2
&&
+ fscanf(fp, "%"SCNuMAX" ", &pid) == 1 &&
+ !strbuf_getwholeline(&locking_host, fp, '\0') &&
/* be gentle to concurrent "gc" on remote hosts */
- (strcmp(locking_host, my_host) || !kill(pid, 0) ||
errno == EPERM);
+ (strcmp(locking_host.buf, my_host) || !kill(pid, 0) ||
errno == EPERM);
if (fp != NULL)
fclose(fp);
if (should_exit) {
@@ -278,7 +278,7 @@ static const char *lock_repo_for_
Re: [PATCH v2] xgethostname: handle long hostnames
Junio C Hamano writes:
> David Turner writes:
>
>> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t*
>> ret_pid)
>> ...
>> if (!force) {
>> -static char locking_host[128];
>> +static char locking_host[HOST_NAME_MAX + 1];
>> int should_exit;
>> fp = fopen(pidfile_path, "r");
>> memset(locking_host, 0, sizeof(locking_host));
>
> I compared the result of applying this v2 directly on top of master
> and applying René's "Use HOST_NAME_MAX"and then applying your v1.
> This hunk is the only difference.
>
> As this locking_host is used like so in the later part of the code:
>
> time(NULL) - st.st_mtime <= 12 * 3600 &&
> fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2
> &&
> /* be gentle to concurrent "gc" on remote hosts */
> (strcmp(locking_host, my_host) || !kill(pid, 0) ||
> errno == EPERM);
>
> I suspect that turning it to HOST_NAME_MAX + 1 without tweaking
> the format "%127c" gives us an inconsistent resulting code.
>
> Of course, my_host is sized to HOST_NAME_MAX + 1 and we are
> comparing it with locking_host, so perhaps we'd need to take this
> version to size locking_host to also HOST_NAME_MAX + 1, and then
> scan with %255c (but then shouldn't we scan with %256c instead? I
> am not sure where these +1 comes from).
That is, something along this line...
builtin/gc.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/builtin/gc.c b/builtin/gc.c
index be75508292..4f85610d87 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -240,7 +240,11 @@ static const char *lock_repo_for_gc(int force, pid_t*
ret_pid)
LOCK_DIE_ON_ERROR);
if (!force) {
static char locking_host[HOST_NAME_MAX + 1];
+ static char *scan_fmt;
int should_exit;
+
+ if (!scan_fmt)
+ scan_fmt = xstrfmt("%s %%%dc", "%"SCNuMAX,
HOST_NAME_MAX);
fp = fopen(pidfile_path, "r");
memset(locking_host, 0, sizeof(locking_host));
should_exit =
@@ -256,7 +260,7 @@ static const char *lock_repo_for_gc(int force, pid_t*
ret_pid)
* running.
*/
time(NULL) - st.st_mtime <= 12 * 3600 &&
- 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)
Re: [PATCH v2] xgethostname: handle long hostnames
David Turner writes:
> @@ -250,14 +250,14 @@ static const char *lock_repo_for_gc(int force, pid_t*
> ret_pid)
> ...
> if (!force) {
> - static char locking_host[128];
> + static char locking_host[HOST_NAME_MAX + 1];
> int should_exit;
> fp = fopen(pidfile_path, "r");
> memset(locking_host, 0, sizeof(locking_host));
I compared the result of applying this v2 directly on top of master
and applying René's "Use HOST_NAME_MAX"and then applying your v1.
This hunk is the only difference.
As this locking_host is used like so in the later part of the code:
time(NULL) - st.st_mtime <= 12 * 3600 &&
fscanf(fp, "%"SCNuMAX" %127c", &pid, locking_host) == 2
&&
/* be gentle to concurrent "gc" on remote hosts */
(strcmp(locking_host, my_host) || !kill(pid, 0) ||
errno == EPERM);
I suspect that turning it to HOST_NAME_MAX + 1 without tweaking
the format "%127c" gives us an inconsistent resulting code.
Of course, my_host is sized to HOST_NAME_MAX + 1 and we are
comparing it with locking_host, so perhaps we'd need to take this
version to size locking_host to also HOST_NAME_MAX + 1, and then
scan with %255c (but then shouldn't we scan with %256c instead? I
am not sure where these +1 comes from).
