Re: [gentoo-portage-dev] [PATCH 3/3] pym/portage/util/locale.py: add a C module to help check locale

[Michał Górny]

On Sun, 29 May 2016 02:37:50 -0400
"Anthony G. Basile" <basile@××××××××××××××.edu> wrote:

> On 5/29/16 2:30 AM, Michał Górny wrote:
> > On Fri, 27 May 2016 10:26:44 -0400
> > "Anthony G. Basile" <basile@××××××××××××××.edu> wrote:
> >
> >> From: "Anthony G. Basile" <blueness@g.o>
> >>
> >> The current method to check for a sane system locale is to use python's
> >> ctypes.util.find_library() to construct a full library path to the
> >> system libc.so and pass that path to ctypes.CDLL() so we can call
> >> toupper() and tolower() directly.  However, this gets bogged down in
> >> implementation details and fails with musl.
> >>
> >> We work around this design flaw in ctypes with a small python module
> >> written in C which provides thin wrappers to toupper() and tolower(),
> >> and only fall back on the current ctypes-based check when this module
> >> is not available.
> >>
> >> This has been tested on glibc, uClibc and musl systems.
> >>
> >> X-Gentoo-bug: 571444
> >> X-Gentoo-bug-url: https://bugs.gentoo.org/show_bug.cgi?id=571444
> >>
> >> Signed-off-by: Anthony G. Basile <blueness@g.o>
> >> ---
> >>  pym/portage/util/locale.py | 16 ++++++-----
> >>  setup.py                   |  6 +++-
> >>  src/portage_util_libc.c    | 70 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 84 insertions(+), 8 deletions(-)
> >>  create mode 100644 src/portage_util_libc.c
> >>
> >> diff --git a/pym/portage/util/locale.py b/pym/portage/util/locale.py
> >> index 093eb86..5b09945 100644
> >> --- a/pym/portage/util/locale.py
> >> +++ b/pym/portage/util/locale.py
> >> @@ -34,13 +34,15 @@ def _check_locale(silent):
> >>  	"""
> >>  	The inner locale check function.
> >>  	"""
> >> -
> >> -	libc_fn = find_library("c")
> >> -	if libc_fn is None:
> >> -		return None
> >> -	libc = LoadLibrary(libc_fn)
> >> -	if libc is None:
> >> -		return None
> >> +	try:
> >> +		from portage.util import libc
> >> +	except ImportError:
> >> +		libc_fn = find_library("c")
> >> +		if libc_fn is None:
> >> +			return None
> >> +		libc = LoadLibrary(libc_fn)
> >> +		if libc is None:
> >> +			return None
> >>
> >>  	lc = list(range(ord('a'), ord('z')+1))
> >>  	uc = list(range(ord('A'), ord('Z')+1))
> >> diff --git a/setup.py b/setup.py
> >> index 25429bc..5ca8156 100755
> >> --- a/setup.py
> >> +++ b/setup.py
> >> @@ -47,7 +47,11 @@ x_scripts = {
> >>  # Dictionary custom modules written in C/C++ here.  The structure is
> >>  #   key   = module name
> >>  #   value = list of C/C++ source code, path relative to top source directory
> >> -x_c_helpers = {}
> >> +x_c_helpers = {
> >> +	'portage.util.libc' : [
> >> +		'src/portage_util_libc.c',
> >> +	],
> >> +}
> >>
> >>  class x_build(build):
> >>  	""" Build command with extra build_man call. """
> >> diff --git a/src/portage_util_libc.c b/src/portage_util_libc.c
> >> new file mode 100644
> >> index 0000000..00b09c2
> >> --- /dev/null
> >> +++ b/src/portage_util_libc.c
> >> @@ -0,0 +1,70 @@
> >> +/* Copyright 2005-2016 Gentoo Foundation
> >> + * Distributed under the terms of the GNU General Public License v2
> >> + */
> >> +
> >> +#include <Python.h>
> >> +#include <stdlib.h>
> >> +#include <ctype.h>
> >> +
> >> +static PyObject * _libc_tolower(PyObject *, PyObject *);
> >> +static PyObject * _libc_toupper(PyObject *, PyObject *);
> >> +
> >> +static PyMethodDef LibcMethods[] = {
> >> +	{"tolower", _libc_tolower, METH_VARARGS, "Convert to lower case using system locale."},
> >> +	{"toupper", _libc_toupper, METH_VARARGS, "Convert to upper case using system locale."},
> >> +	{NULL, NULL, 0, NULL}
> >> +};
> >> +
> >> +#if PY_MAJOR_VERSION >= 3
> >> +static struct PyModuleDef moduledef = {
> >> +	PyModuleDef_HEAD_INIT,
> >> +	"libc",								/* m_name */
> >> +	"Module for converting case using the system locale",		/* m_doc */
> >> +	-1,								/* m_size */
> >> +	LibcMethods,							/* m_methods */
> >> +	NULL,								/* m_reload */
> >> +	NULL,								/* m_traverse */
> >> +	NULL,								/* m_clear */
> >> +	NULL,								/* m_free */
> >> +};
> >> +#endif
> >> +
> >> +PyMODINIT_FUNC
> >
> > Now you could call it nitpicking but since it decorates the function,
> > it would be better to have it inside the #ifdef. Otherwise, people
> > would think it's separate from that.
>
> You mean repeat it twice?

Yes. Otherwise it makes people think it outputs a separate block
independent of the functions.

> >> +
> >> +#if PY_MAJOR_VERSION >= 3
> >> +PyInit_libc(void)
> >> +{
> >> +	PyObject *m;
> >> +	m = PyModule_Create(&moduledef);
> >> +	return m;
> >> +}
> >> +#else
> >> +initlibc(void)
> >> +{
> >> +	Py_InitModule("libc", LibcMethods);
> >> +}
> >> +#endif
> >> +
> >> +
> >> +static PyObject *
> >> +_libc_tolower(PyObject *self, PyObject *args)
> >> +{
> >> +	int c;
> >> +
> >> +	if (!PyArg_ParseTuple(args, "i", &c))
> >> +		return NULL;
> >> +
> >> +	return Py_BuildValue("i", tolower(c));
> >> +}
> >> +
> >> +
> >> +static PyObject *
> >> +_libc_toupper(PyObject *self, PyObject *args)
> >> +{
> >> +	int c;
> >> +
> >> +	if (!PyArg_ParseTuple(args, "i", &c))
> >> +		return NULL;
> >> +
> >> +	return Py_BuildValue("i", toupper(c));
> >> +}
> >
> > Aside from that, it look nice and shiny now. Thanks a lot!
>
> so what's the other stuff that could get put into this module that you
> mentioned?

I'd go for all uses of find_library("C").

pym/portage/dbapi/_MergeProcess.py + pym/portage/dbapi/_SyncfsProcess.py
use it to do syncfs(). You'd probably have to do more research but I
think we could even get rid of the forking there.

pym/portage/process.py uses it for unshare() call. There are also some kernel
constants for ioctls hardcoded. It would be great to move both unshare()
and the constants to the module, so they are picked from the headers.

-- 
Best regards,
Michał Górny
<http://dev.gentoo.org/~mgorny/>