Re: [PATCH 1/3] Linux: Add close_range

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella:

>> I think we generally use int for file descriptors, following POSIX.
>
> The Linux interface uses unsigned integer, meaning that negative values
> won't really trigger an invalid usage (kernel will return EINVAL if 
> second argument is larger than first one though).
>
> I would prefer for syscall wrapper to be close as possible of the kernel
> interface, otherwise it would require to add additional semantic to
> handle potential pitfall cases.
>
> On this case, if we go for 'int' as argument should we return EBADF 
> for invalid handles?

Hmm.  I think unsigned int is needed for ~0U to work, which is what you
used in the tests.  If that's what applications use today when issuing
the syscall directly, I think we need to stick to unsigned int.

>> __THROW is incompatible with close_range as a cancellation point.  I'm
>> not sure if it is useful for this function to be a cancellation point,
>> given that it's mostly used for cleanup.  It's true that close can block
>> for an indefinite time for certain descriptors, but I don't know if
>> close_range inherits that behavior.  It certainly does not apply to
>> CLOSE_RANGE_CLOEXEC.
>
> I wasn't sure about making a cancellation entrypoint, so I followed
> current close semantic.  The cancellation entrypoing would act also
> in an early mode, meaning that asynchronous cancellation is enabled
> and thread signaled the cancellation will act prior the syscall 
> execution. 
>
> In the other hand, reading the close_range code it does seen it 
> returns with side-effects due signal interrupt. So maybe not making
> a cancellation entrypoint is indeed a better option.
>
> The FreeBSD implementation does not seem to make close_range a 
> cancellation entrypoint.

Yes, let's do not act upon cancellation.

>>> diff --git a/sysdeps/unix/sysv/linux/tst-clone.h b/sysdeps/unix/sysv/linux/tst-clone.h
>>> new file mode 100644
>>> index 0000000000..2344ac5101
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/tst-clone.h
>>> @@ -0,0 +1,48 @@
>>> +/* Auxiliary wrapper to issue the clone symbol.
>> 
>> It's not clear to me what this means.
>
> Maybe:
>
>   Auxiliary functions to issue the clone syscall. ?

That's better, thanks.

>>> diff --git a/sysdeps/unix/sysv/linux/tst-close_range.c b/sysdeps/unix/sysv/linux/tst-close_range.c
>>> new file mode 100644
>>> index 0000000000..a685e7d359
>>> --- /dev/null
>>> +++ b/sysdeps/unix/sysv/linux/tst-close_range.c
>> 
>>> +static unsigned int
>>> +first_invalid_flag (void)
>>> +{
>>> +  static const unsigned int flags[] = {
>>> +    CLOSE_RANGE_UNSHARE,
>>> +  };
>>> +
>>> +  unsigned int r = 1;
>>> +  for (int i = 0; i < array_length (flags); i++)
>>> +    if ((1U << r) & flags[i])
>>> +      r++;
>>> +  return 1U << r;
>>> +}
>>> +
>>> +enum
>>> +{
>>> +  maximum_fd = 100,
>>> +  half_fd = maximum_fd / 2,
>>> +  gap_1 = maximum_fd - 8,
>>> +  gap_0 = (maximum_fd * 3) / 4
>>> +};
>>> +
>>> +static void
>>> +close_range_test_common (int *fds, unsigned int flags)
>>> +{
>>> +  /* Basic test for invalid flags, bail out if unsupported.  */
>>> +  TEST_COMPARE (close_range (fds[0], fds[half_fd], first_invalid_flag()), -1);
>> 
>> This test is not future-proof at all.  I don't think it's valuable.
>
> But the idea is exactly to raise a warning if the underlying kernel
> supports more flags than the one support by glibc.

But it causes a test failure, not a warning.  I don't think it's valid
to poke for unknown system call flags like this, so even a proper
warning would be hard.

>>> +  if (errno == ENOSYS)
>>> +    FAIL_UNSUPPORTED ("close_range not supported");
>>> +  TEST_COMPARE (errno, EINVAL);
>>> +
>>> +  /* Close half of the descriptors and check result.  */
>>> +  TEST_COMPARE (close_range (fds[0], fds[half_fd], flags), 0);
>>> +  for (int i = 0; i <= half_fd; i++)
>>> +    {
>>> +      TEST_COMPARE (fcntl (fds[i], F_GETFL), -1);
>>> +      TEST_COMPARE (errno, EBADF);
>>> +    }
>> 
>> I have some difficulty figuring out whether you expect the descriptors
>> to be in a continuous range or not.
>> 
>> I think it would make sense to create one specific layout of file
>> descriptors and test with that.
>
> I think it is a fair assumption that descriptors are a continuous range.
> I modeled this tests using the kernel one as base, which make the same
> assumptions [1]
>
> [1] tools/testing/selftests/core/close_range_test.c

The you don't need the fds array.  It's what makes this confusing.

>>> +static int
>>> +close_range_unshare_test_fn (void *arg)
>>> +{
>>> +  close_range_test_common (arg, CLOSE_RANGE_UNSHARE);
>>> +  exit (EXIT_SUCCESS);
>>> +}
>>> +
>>> +/* Check if a close_range with CLOSE_RANGE_UNSHARE issued from a subprocess
>>> +   created with CLONE_FILES does not close the parent file descriptor list.  */
>>> +static void
>>> +close_range_unshare_test (void)
>>> +{
>> 
>> I think there could also be a test without CLOSE_RANGE_UNSHARE that
>> verifies that the subprocess operators on the shared file descriptor
>> table.
>
> Do you mean issue a 	? I may add it, although it would tests
> more CLONE_FILES support than close_range.

Sorry?  I meant a test that shows that flags == 0 and flags ==
CLOSE_RANGE_UNSHARE behave differently.

Thanks,
Florian
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill