Skip to content

Conversation

@rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Sep 24, 2024

What's confusing me now is that it seems there's no equivalent to getdtablesize on Windows. If it's allowed, I could write an API to provide an estimated value for the Windows system.

And I'm a bit confused about the clinic tag. This is my first time submitting changes to the C-API. Could you point out any mistakes I might have made? Thanks!!

@rruuaanng rruuaanng changed the title Added getdtablesize function to os module gh-83923: Added getdtablesize function to os module Sep 24, 2024
@rruuaanng rruuaanng changed the title gh-83923: Added getdtablesize function to os module gh-83923: Added os.getdtablesize function to os module Sep 24, 2024
@rruuaanng
Copy link
Contributor Author

cc @skirpichev

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Sep 24, 2024

I think the failure of document builind isn't important, because it is a C extension. So there is no Python definition.

@skirpichev
Copy link
Contributor

No, it is. CI status should be green.

You should document new functions in sphinx docs as well.

@rruuaanng
Copy link
Contributor Author

No, it is. CI status should be green.

You should document new functions in sphinx docs as well.

Okay, I'm adding os related documents, and eliminating CI failed.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds interesting to expose getdtablesize() in Python.

@vstinner
Copy link
Member

It seems like the function is available on FreeBSD, macOS and Solaris, but not on OpenBSD nor NetBSD.

@vstinner
Copy link
Member

It seems like the function is available on FreeBSD, macOS and Solaris, but not on OpenBSD nor NetBSD.

You should check if the function is available by adding getdtablesize to configure.ac in this list:

# checks for library functions
AC_CHECK_FUNCS([ \
  accept4 alarm bind_textdomain_codeset chmod chown clock closefrom close_range
  copy_file_range ctermid dup dup3 execv explicit_bzero explicit_memset \
  faccessat fchmod fchmodat fchown fchownat fdopendir fdwalk fexecve \
  ...

And then run make regen-configure.

@rruuaanng
Copy link
Contributor Author

Okay, Thanks for pointing out my mistake, and thanks for your review!

@vstinner
Copy link
Member

@serhiy-storchaka @corona10 @erlend-aasland: Do you think that it's worth it to expose getdtablesize() in the os module? The Linux manual page says:

Portable applications should employ sysconf(_SC_OPEN_MAX) instead of this call.

os.sysconf("SC_OPEN_MAX") is already available on Python.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Sep 24, 2024

In #83923

getdtablesize({2,3}) is a wonderful library call/system call to have access to because it allows one to determine the descriptor limits at runtime in an easier manner than having to do the equivalent with os.sysconf(..)

I think this‘s necessary, and intuitive.

@serhiy-storchaka
Copy link
Member

You can also use resource.getrlimit(resource.RLIMIT_NOFILE).

I do not think that we need to implement a third way.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Sep 24, 2024

You can also use resource.getrlimit(resource.RLIMIT_NOFILE).

I do not think that we need to implement a third way.

If you're familiar with POSIX but not with Python, this can be pretty tough for you. Like me, for example.

@serhiy-storchaka
Copy link
Member

getdtablesize() is not in POSIX, unlike sysconf() and getrlimit(). This is another argument not to add it.

@rruuaanng
Copy link
Contributor Author

getdtablesize() is not in POSIX, unlike sysconf() and getrlimit(). This is another argument not to add it.

Oh sorry, it should be BSD. Just like #83923, It seems like nothing else works as well as calling someone by their name when it comes to making them feel good.

@rruuaanng
Copy link
Contributor Author

rruuaanng commented Sep 24, 2024

Because of #ifndef MS_WINDOWS, CI appears declared but not defined. Maybe I should move #ifndef MS_WINDOWS into the os_getdtablesize_impl.

D:\a\cpython\cpython\Modules\clinic\posixmodule.c.h(1432,1): error C2129: static function 'PyObject *os_getdtablesize_impl(PyObject *)' declared but not defined [D:\a\cpython\cpython\PCbuild\_freeze_module.vcxproj]
         (compiling source file '../Modules/posixmodule.c')
             D:\a\cpython\cpython\Modules\clinic\posixmodule.c.h(1432,1):
             see declaration of 'os_getdtablesize_impl'
/*[clinic input]
os.getdtablesize

Return the maximum number of files a process can have open.
[clinic start generated code]*/

static PyObject *
os_getdtablesize_impl(PyObject *module)
{
    int size;
#ifndef MS_WINDOWS
    size = getdtablesize();
#endif  /* !MS_WINDOWS */
    return PyLong_FromLong(size);
}

or

#ifndef MS_WINDOWS
...
#else
static PyObject *
os_getdtablesize_impl(PyObject *Py_UNUSED(module)) { Py_RETURN_NONE; }
#endif  /* !MS_WINDOWS */

@rruuaanng rruuaanng requested a review from vstinner September 24, 2024 14:45
@vstinner
Copy link
Member

@rruuaanng: I suggest you to pause the implementation until we decide if we should add the function or not.

@rruuaanng
Copy link
Contributor Author

@rruuaanng: I suggest you to pause the implementation until we decide if we should add the function or not.

Okay, I think so.

@vstinner
Copy link
Member

@serhiy-storchaka:

getdtablesize() is not in POSIX, unlike sysconf() and getrlimit(). This is another argument not to add it.

The glibc implements getdtablesize() by calling getrlimit(RLIMIT_NOFILE) on POSIX.

Let's close the issue, I agree with @serhiy-storchaka that this function is redundant and it's not worth it to add it. Sorry @rruuaanng. I close the issue.

@vstinner vstinner closed this Sep 26, 2024
@rruuaanng rruuaanng deleted the gh83923 branch September 26, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants