diff mbox series

posix/tst-spawn5 failes under make -j

Message ID 87czrrizw0.fsf@oldenburg.str.redhat.com
State Not applicable
Headers
Series posix/tst-spawn5 failes under make -j |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

Florian Weimer July 9, 2021, 9:45 a.m. UTC 
  This patch adds some additional diagonstics:

It's the pipe from the make job server:

error: tst-spawn5.c:122: unexpected open file descriptor 3: pipe:[9958839]

I think the test needs to be changed so that lowfd is passed across the
restart, and checking only considers descriptors >= lowfd.

Comments?

Thanks,
Florian

Comments

Adhemerval Zanella July 9, 2021, 1:10 p.m. UTC | #1 
On 09/07/2021 06:45, Florian Weimer wrote:
> This patch adds some additional diagonstics:
> 
> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
> index 277b848794..5ba7c7fadb 100644
> --- a/posix/tst-spawn5.c
> +++ b/posix/tst-spawn5.c
> @@ -116,7 +116,11 @@ handle_restart (int argc, char *argv[])
>  	  fds[i].found = found = true;
>  
>        if (!found)
> -        FAIL_EXIT1 ("unexpected open file descriptor: %ld", fd);
> +	{
> +	  char *path = xasprintf ("/proc/self/fd/%s", e->d_name);
> +	  char *resolved = xreadlink (path);
> +	  FAIL_EXIT1 ("unexpected open file descriptor %ld: %s", fd, resolved);
> +	}
>      }
>    closedir (dirp);
>  
> It's the pipe from the make job server:
> 
> error: tst-spawn5.c:122: unexpected open file descriptor 3: pipe:[9958839]
> 
> I think the test needs to be changed so that lowfd is passed across the
> restart, and checking only considers descriptors >= lowfd.

The restart process already receive a list of the expected opened files,
for instance on my environment it will re-executed with:

handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
handle_restart: 3 
handle_restart: 
handle_restart: 3 4 

I am trying to reproduce the issue without much success, even with 
make -j or make posix/tests -j.  Maybe we can limit the search to the
argument range:

diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
index e04a6a8088..a9a3aa5d8d 100644
--- a/posix/tst-spawn5.c
+++ b/posix/tst-spawn5.c
@@ -68,6 +68,8 @@ handle_restart (int argc, char *argv[])
     int fd;
     _Bool found;
   } *fds = xmalloc (sizeof (struct fd_t) * nfds);
+  int maxfd = 0;
+  int minfd = INT_MAX;
   for (int i = 0; i < nfds; i++)
     {
       char *endptr;
@@ -77,6 +79,11 @@ handle_restart (int argc, char *argv[])
 
       fds[i].fd = fd;
       fds[i].found = false;
+
+      if (fd < minfd)
+       minfd = fd;
+      if (fd > maxfd)
+       maxfd = fd;
     }
 
   DIR *dirp = opendir (FD_TO_FILENAME_PREFIX);
@@ -109,6 +116,8 @@ handle_restart (int argc, char *argv[])
          || fd == STDOUT_FILENO
          || fd == STDERR_FILENO)
         continue;
+      if (fd < minfd || fd > maxfd)
+        continue;
 
       bool found = false;
       for (int i = 0; i < nfds; i++)
Florian Weimer July 9, 2021, 1:41 p.m. UTC | #2 
* Adhemerval Zanella:

> On 09/07/2021 06:45, Florian Weimer wrote:
>> This patch adds some additional diagonstics:
>> 
>> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
>> index 277b848794..5ba7c7fadb 100644
>> --- a/posix/tst-spawn5.c
>> +++ b/posix/tst-spawn5.c
>> @@ -116,7 +116,11 @@ handle_restart (int argc, char *argv[])
>>  	  fds[i].found = found = true;
>>  
>>        if (!found)
>> -        FAIL_EXIT1 ("unexpected open file descriptor: %ld", fd);
>> +	{
>> +	  char *path = xasprintf ("/proc/self/fd/%s", e->d_name);
>> +	  char *resolved = xreadlink (path);
>> +	  FAIL_EXIT1 ("unexpected open file descriptor %ld: %s", fd, resolved);
>> +	}
>>      }
>>    closedir (dirp);
>>  
>> It's the pipe from the make job server:
>> 
>> error: tst-spawn5.c:122: unexpected open file descriptor 3: pipe:[9958839]
>> 
>> I think the test needs to be changed so that lowfd is passed across the
>> restart, and checking only considers descriptors >= lowfd.
>
> The restart process already receive a list of the expected opened files,
> for instance on my environment it will re-executed with:
>
> handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
> handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
> handle_restart: 3 
> handle_restart: 
> handle_restart: 3 4 
>
> I am trying to reproduce the issue without much success, even with 
> make -j or make posix/tests -j.  Maybe we can limit the search to the
> argument range:

make -j2 should do it (make -j without argument probably doesn't involve
the job server), sorry.

The issue is that the verification should start only at lowfd.
This will supercede the STDIN_* checks here:

      if (fd == dirfd (dirp)
          || fd == STDIN_FILENO
	  || fd == STDOUT_FILENO
	  || fd == STDERR_FILENO)
        continue;


Thanks,
Florian
Adhemerval Zanella July 9, 2021, 2:17 p.m. UTC | #3 
On 09/07/2021 10:41, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 09/07/2021 06:45, Florian Weimer wrote:
>>> This patch adds some additional diagonstics:
>>>
>>> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
>>> index 277b848794..5ba7c7fadb 100644
>>> --- a/posix/tst-spawn5.c
>>> +++ b/posix/tst-spawn5.c
>>> @@ -116,7 +116,11 @@ handle_restart (int argc, char *argv[])
>>>  	  fds[i].found = found = true;
>>>  
>>>        if (!found)
>>> -        FAIL_EXIT1 ("unexpected open file descriptor: %ld", fd);
>>> +	{
>>> +	  char *path = xasprintf ("/proc/self/fd/%s", e->d_name);
>>> +	  char *resolved = xreadlink (path);
>>> +	  FAIL_EXIT1 ("unexpected open file descriptor %ld: %s", fd, resolved);
>>> +	}
>>>      }
>>>    closedir (dirp);
>>>  
>>> It's the pipe from the make job server:
>>>
>>> error: tst-spawn5.c:122: unexpected open file descriptor 3: pipe:[9958839]
>>>
>>> I think the test needs to be changed so that lowfd is passed across the
>>> restart, and checking only considers descriptors >= lowfd.
>>
>> The restart process already receive a list of the expected opened files,
>> for instance on my environment it will re-executed with:
>>
>> handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
>> handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
>> handle_restart: 3 
>> handle_restart: 
>> handle_restart: 3 4 
>>
>> I am trying to reproduce the issue without much success, even with 
>> make -j or make posix/tests -j.  Maybe we can limit the search to the
>> argument range:
> 
> make -j2 should do it (make -j without argument probably doesn't involve
> the job server), sorry.
> 
> The issue is that the verification should start only at lowfd.
> This will supercede the STDIN_* checks here:
> 
>       if (fd == dirfd (dirp)
>           || fd == STDIN_FILENO
> 	  || fd == STDOUT_FILENO
> 	  || fd == STDERR_FILENO)
>         continue;

I usually ran the check with the current processor number in my system
in fact (in this case 24) and I haven't see any issue so far.  But I 
think the following patch should fine, can you check if it improves the
error you are seeing:

diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
index 277b848794..d586662a8e 100644
--- a/posix/tst-spawn5.c
+++ b/posix/tst-spawn5.c
@@ -68,6 +68,8 @@ handle_restart (int argc, char *argv[])
     int fd;
     _Bool found;
   } *fds = xmalloc (sizeof (struct fd_t) * nfds);
+  int maxfd = 0;
+  int minfd = INT_MAX;
   for (int i = 0; i < nfds; i++)
     {
       char *endptr;
@@ -77,6 +79,11 @@ handle_restart (int argc, char *argv[])
 
       fds[i].fd = fd;
       fds[i].found = false;
+
+      if (fd < minfd)
+       minfd = fd;
+      if (fd > maxfd)
+       maxfd = fd;
     }
 
   DIR *dirp = opendir (FD_TO_FILENAME_PREFIX);
@@ -103,11 +110,8 @@ handle_restart (int argc, char *argv[])
         FAIL_EXIT1 ("readdir: invalid file descriptor name: /proc/self/fd/%s",
                     e->d_name);
 
-      /* Skip the descriptor which is used to enumerate the descriptors.  */
-      if (fd == dirfd (dirp)
-          || fd == STDIN_FILENO
-         || fd == STDOUT_FILENO
-         || fd == STDERR_FILENO)
+      /* Skip the descriptor which are not enumerated in arguments.  */
+      if (fd < minfd || fd > maxfd)
         continue;
 
       bool found = false;
Florian Weimer July 9, 2021, 6:40 p.m. UTC | #4 
* Adhemerval Zanella:

> On 09/07/2021 10:41, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 09/07/2021 06:45, Florian Weimer wrote:
>>>> This patch adds some additional diagonstics:
>>>>
>>>> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
>>>> index 277b848794..5ba7c7fadb 100644
>>>> --- a/posix/tst-spawn5.c
>>>> +++ b/posix/tst-spawn5.c
>>>> @@ -116,7 +116,11 @@ handle_restart (int argc, char *argv[])
>>>>  	  fds[i].found = found = true;
>>>>  
>>>>        if (!found)
>>>> -        FAIL_EXIT1 ("unexpected open file descriptor: %ld", fd);
>>>> +	{
>>>> +	  char *path = xasprintf ("/proc/self/fd/%s", e->d_name);
>>>> +	  char *resolved = xreadlink (path);
>>>> +	  FAIL_EXIT1 ("unexpected open file descriptor %ld: %s", fd, resolved);
>>>> +	}
>>>>      }
>>>>    closedir (dirp);
>>>>  
>>>> It's the pipe from the make job server:
>>>>
>>>> error: tst-spawn5.c:122: unexpected open file descriptor 3: pipe:[9958839]
>>>>
>>>> I think the test needs to be changed so that lowfd is passed across the
>>>> restart, and checking only considers descriptors >= lowfd.
>>>
>>> The restart process already receive a list of the expected opened files,
>>> for instance on my environment it will re-executed with:
>>>
>>> handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
>>> handle_restart: 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 
>>> handle_restart: 3 
>>> handle_restart: 
>>> handle_restart: 3 4 
>>>
>>> I am trying to reproduce the issue without much success, even with 
>>> make -j or make posix/tests -j.  Maybe we can limit the search to the
>>> argument range:
>> 
>> make -j2 should do it (make -j without argument probably doesn't involve
>> the job server), sorry.
>> 
>> The issue is that the verification should start only at lowfd.
>> This will supercede the STDIN_* checks here:
>> 
>>       if (fd == dirfd (dirp)
>>           || fd == STDIN_FILENO
>> 	  || fd == STDOUT_FILENO
>> 	  || fd == STDERR_FILENO)
>>         continue;
>
> I usually ran the check with the current processor number in my system
> in fact (in this case 24) and I haven't see any issue so far.  But I 
> think the following patch should fine, can you check if it improves the
> error you are seeing:

The issue is that the test range (lowfd) starts at 4 in my case, while
the verification logic assumes it's effectively 3.

>
> diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
> index 277b848794..d586662a8e 100644
> --- a/posix/tst-spawn5.c
> +++ b/posix/tst-spawn5.c
> @@ -68,6 +68,8 @@ handle_restart (int argc, char *argv[])
>      int fd;
>      _Bool found;
>    } *fds = xmalloc (sizeof (struct fd_t) * nfds);
> +  int maxfd = 0;
> +  int minfd = INT_MAX;
>    for (int i = 0; i < nfds; i++)
>      {
>        char *endptr;
> @@ -77,6 +79,11 @@ handle_restart (int argc, char *argv[])
>  
>        fds[i].fd = fd;
>        fds[i].found = false;
> +
> +      if (fd < minfd)
> +       minfd = fd;
> +      if (fd > maxfd)
> +       maxfd = fd;
>      }

I think this interferes with the objective of the test.  You really need
to pass lowfd to the target process.

Thanks,
Florian
diff mbox series

Patch

diff --git a/posix/tst-spawn5.c b/posix/tst-spawn5.c
index 277b848794..5ba7c7fadb 100644
--- a/posix/tst-spawn5.c
+++ b/posix/tst-spawn5.c
@@ -116,7 +116,11 @@  handle_restart (int argc, char *argv[])
 	  fds[i].found = found = true;
 
       if (!found)
-        FAIL_EXIT1 ("unexpected open file descriptor: %ld", fd);
+	{
+	  char *path = xasprintf ("/proc/self/fd/%s", e->d_name);
+	  char *resolved = xreadlink (path);
+	  FAIL_EXIT1 ("unexpected open file descriptor %ld: %s", fd, resolved);
+	}
     }
   closedir (dirp);