* Z/OS Enhancement to gnu lib @ 2023-01-30 4:09 Igor Todorovski 2023-01-30 11:48 ` Bruno Haible 0 siblings, 1 reply; 4+ messages in thread From: Igor Todorovski @ 2023-01-30 4:09 UTC (permalink / raw) To: bug-gnulib@gnu.org, Mike Fulton [-- Attachment #1: Type: text/plain, Size: 1924 bytes --] Hi, I was wondering if the following changes I made to findutils (to get it to function on z/OS) can be merged into gnulib. The first change (fdopendir.c) guards the close call. Otherwise we get a bad file descriptor on z/OS. I am not sure if this has any other consequences, but so far I haven’t seen any issues with findutils. The second change (openat-proc.c) adds a way to get the pathname when given a file descriptor as an input. If you have any comments, please let me know: index c2b0e1e..82ae2e4 100644 --- a/gl/lib/fdopendir.c +++ b/gl/lib/fdopendir.c @@ -151,7 +151,9 @@ fdopendir_with_dup (int fd, int older_dupfd, struct saved_cwd const *cwd) } else { +#ifndef __MVS__ close (fd); +#endif dir = fd_clone_opendir (dupfd, cwd); saved_errno = errno; if (! dir) diff --git a/gl/lib/openat-proc.c b/gl/lib/openat-proc.c index 3bacf7d..bb788fd 100644 --- a/gl/lib/openat-proc.c +++ b/gl/lib/openat-proc.c @@ -28,6 +28,7 @@ #include <stdio.h> #include <stdlib.h> #include <string.h> +#include <termios.h> #include <unistd.h> #ifdef __KLIBC__ @@ -53,7 +54,27 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) return buf; } -#ifndef __KLIBC__ +#ifdef __MVS__ + { + char dir[_XOPEN_PATH_MAX]; + int rc = w_ioctl(fd, _IOCC_GPN, _XOPEN_PATH_MAX, dir); + if (rc == 0) { + __e2a_l(dir, _XOPEN_PATH_MAX); + } + size_t bufsize; + dirlen = strlen (dir); + bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */ + if (OPENAT_BUFFER_SIZE < bufsize) + { + result = malloc (bufsize); + if (! result) + return NULL; + } + + strcpy (result, dir); + result[dirlen++] = '/'; + } +#elif !defined( __KLIBC__) # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/" { enum { [-- Attachment #2: Type: text/html, Size: 14454 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Z/OS Enhancement to gnu lib 2023-01-30 4:09 Z/OS Enhancement to gnu lib Igor Todorovski @ 2023-01-30 11:48 ` Bruno Haible 2023-01-30 17:29 ` Igor Todorovski 0 siblings, 1 reply; 4+ messages in thread From: Bruno Haible @ 2023-01-30 11:48 UTC (permalink / raw) To: bug-gnulib, Igor Todorovski; +Cc: Mike Fulton Hi, > I was wondering if the following changes I made to findutils (to get it to function on z/OS) can be merged into gnulib. > > The first change (fdopendir.c) guards the close call. Otherwise we get a bad file descriptor on z/OS. What's the problem with that "bad file descriptor" error? On POSIX system, EBADF is the natural error code in this situation. If close() on z/OS does not support this situation gracefully, we'll need to override the close() function, like we did on native Windows, see <https://www.gnu.org/software/gnulib/manual/html_node/close.html>. > I am not sure if this has any other consequences, but so far I haven’t seen any issues with findutils. If fd is not invalid, it would lead to a file descriptor leak, no? > The second change (openat-proc.c) adds a way to get the pathname when given a file descriptor as an input. Thanks. This looks reasonable. > diff --git a/gl/lib/openat-proc.c b/gl/lib/openat-proc.c > index 3bacf7d..bb788fd 100644 > --- a/gl/lib/openat-proc.c > +++ b/gl/lib/openat-proc.c > @@ -28,6 +28,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <termios.h> > #include <unistd.h> > #ifdef __KLIBC__ > @@ -53,7 +54,27 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) > return buf; > } > -#ifndef __KLIBC__ > +#ifdef __MVS__ > + { > + char dir[_XOPEN_PATH_MAX]; > + int rc = w_ioctl(fd, _IOCC_GPN, _XOPEN_PATH_MAX, dir); > + if (rc == 0) { > + __e2a_l(dir, _XOPEN_PATH_MAX); > + } > + size_t bufsize; > + dirlen = strlen (dir); > + bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */ > + if (OPENAT_BUFFER_SIZE < bufsize) > + { > + result = malloc (bufsize); > + if (! result) > + return NULL; > + } > + > + strcpy (result, dir); > + result[dirlen++] = '/'; > + } > +#elif !defined( __KLIBC__) > # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/" > { > enum { > There are a number of things that can be improved from this patch: - The <termios.h> include should only be done on platforms that need it, since not all platforms have <termios.h>. - GNU coding style: placement of braces, space between function name and argument list. - Put special cases for special operating systems after the generic/Linux case. Simply as a convenience, so that the reader does not turn away before getting to the main code. - Avoid code duplication of 12 lines of code, if it can easily be avoided. - Use 'sizeof dir' instead of '_XOPEN_PATH_MAX', so that the dimension of that array needs to be stated only once. Useful for future maintenance. I did these improvements. What I then take from your patch is only 3 lines of code. This does not require a copyright assigment from you to the FSF (cf. <https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html>). Therefore I am incorporating this code directly. Btw, do you have reference documentation available for _IOCC_GPN and __e2a_l? It generally is more advisable to use documented facilities than undocumented ones. 2023-01-30 Bruno Haible <bruno@clisp.org> at-internal: Add support for z/OS. Reported and draft patch by Igor Todorovski <itodorov@ca.ibm.com>. * lib/openat-proc.c [z/OS]: Include <termios.h>. (openat_proc_name): For z/OS, use an approach similar to kLIBC, with 3 lines of z/OS specific code by Igor Todorovski <itodorov@ca.ibm.com>. diff --git a/lib/openat-proc.c b/lib/openat-proc.c index 2a6a85f069..6419a8cf5f 100644 --- a/lib/openat-proc.c +++ b/lib/openat-proc.c @@ -30,9 +30,12 @@ #include <string.h> #include <unistd.h> -#ifdef __KLIBC__ +#ifdef __KLIBC__ /* OS/2 */ # include <InnoTekLIBC/backend.h> #endif +#ifdef __MVS__ /* z/OS */ +# include <termios.h> +#endif #include "intprops.h" @@ -53,7 +56,8 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) return buf; } -#ifndef __KLIBC__ +#if !(defined __KLIBC__ || defined __MVS__) + /* Generic code for Linux, Solaris, and similar platforms. */ # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/" { enum { @@ -107,14 +111,21 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) dirlen = sprintf (result, PROC_SELF_FD_FORMAT, fd); } } -#else +#else /* (defined __KLIBC__ || defined __MVS__), i.e. OS/2 or z/OS */ /* OS/2 kLIBC provides a function to retrieve a path from a fd. */ { - char dir[_MAX_PATH]; size_t bufsize; +# ifdef __KLIBC__ + char dir[_MAX_PATH]; if (__libc_Back_ioFHToPath (fd, dir, sizeof dir)) return NULL; +# endif +# ifdef __MVS__ + char dir[_XOPEN_PATH_MAX]; + if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) == 0) + __e2a_l (dir, sizeof dir); +# endif dirlen = strlen (dir); bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */ ^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: Z/OS Enhancement to gnu lib 2023-01-30 11:48 ` Bruno Haible @ 2023-01-30 17:29 ` Igor Todorovski 2023-01-30 22:12 ` Bruno Haible 0 siblings, 1 reply; 4+ messages in thread From: Igor Todorovski @ 2023-01-30 17:29 UTC (permalink / raw) To: Bruno Haible, bug-gnulib@gnu.org; +Cc: Mike Fulton [-- Attachment #1: Type: text/plain, Size: 6139 bytes --] This was the symptom we experienced with findutils (guarding the close in gnulib fixed it): find . -name "test" The output is: find: '.': EDC5113I Bad file descriptor. https://github.com/ZOSOpenTools/findutilsport/issues/4 We’ll do some more testing to see if there’s a better solution. Yes, there’s a chance it will lead to a leak in file descriptors. __e2a_l is documented here: https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-e2a-l-convert-characters-from-ebcdic-ascii Unfortunately, _IOCC_GPN is not documented, but is in the exposed header files: /usr/include/termios.h: #define _IOCC_GPN 17 /* LFS Get pathname */ I’ll check with the OS team to see if it can be documented. Thank you for making the improvements! Thanks, Igor From: Bruno Haible <bruno@clisp.org> Date: Monday, January 30, 2023 at 6:48 AM To: bug-gnulib@gnu.org <bug-gnulib@gnu.org>, Igor Todorovski <itodorov@ca.ibm.com> Cc: Mike Fulton <fultonm@ca.ibm.com> Subject: [EXTERNAL] Re: Z/OS Enhancement to gnu lib Hi, > I was wondering if the following changes I made to findutils (to get it to function on z/OS) can be merged into gnulib. > > The first change (fdopendir.c) guards the close call. Otherwise we get a bad file descriptor on z/OS. What's the problem with that "bad file descriptor" error? On POSIX system, EBADF is the natural error code in this situation. If close() on z/OS does not support this situation gracefully, we'll need to override the close() function, like we did on native Windows, see <https://www.gnu.org/software/gnulib/manual/html_node/close.html >. > I am not sure if this has any other consequences, but so far I haven’t seen any issues with findutils. If fd is not invalid, it would lead to a file descriptor leak, no? > The second change (openat-proc.c) adds a way to get the pathname when given a file descriptor as an input. Thanks. This looks reasonable. > diff --git a/gl/lib/openat-proc.c b/gl/lib/openat-proc.c > index 3bacf7d..bb788fd 100644 > --- a/gl/lib/openat-proc.c > +++ b/gl/lib/openat-proc.c > @@ -28,6 +28,7 @@ > #include <stdio.h> > #include <stdlib.h> > #include <string.h> > +#include <termios.h> > #include <unistd.h> > #ifdef __KLIBC__ > @@ -53,7 +54,27 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) > return buf; > } > -#ifndef __KLIBC__ > +#ifdef __MVS__ > + { > + char dir[_XOPEN_PATH_MAX]; > + int rc = w_ioctl(fd, _IOCC_GPN, _XOPEN_PATH_MAX, dir); > + if (rc == 0) { > + __e2a_l(dir, _XOPEN_PATH_MAX); > + } > + size_t bufsize; > + dirlen = strlen (dir); > + bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */ > + if (OPENAT_BUFFER_SIZE < bufsize) > + { > + result = malloc (bufsize); > + if (! result) > + return NULL; > + } > + > + strcpy (result, dir); > + result[dirlen++] = '/'; > + } > +#elif !defined( __KLIBC__) > # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/" > { > enum { > There are a number of things that can be improved from this patch: - The <termios.h> include should only be done on platforms that need it, since not all platforms have <termios.h>. - GNU coding style: placement of braces, space between function name and argument list. - Put special cases for special operating systems after the generic/Linux case. Simply as a convenience, so that the reader does not turn away before getting to the main code. - Avoid code duplication of 12 lines of code, if it can easily be avoided. - Use 'sizeof dir' instead of '_XOPEN_PATH_MAX', so that the dimension of that array needs to be stated only once. Useful for future maintenance. I did these improvements. What I then take from your patch is only 3 lines of code. This does not require a copyright assigment from you to the FSF (cf. <https://www.gnu.org/prep/maintain/html_node/Legally-Significant.html >). Therefore I am incorporating this code directly. Btw, do you have reference documentation available for _IOCC_GPN and __e2a_l? It generally is more advisable to use documented facilities than undocumented ones. 2023-01-30 Bruno Haible <bruno@clisp.org> at-internal: Add support for z/OS. Reported and draft patch by Igor Todorovski <itodorov@ca.ibm.com>. * lib/openat-proc.c [z/OS]: Include <termios.h>. (openat_proc_name): For z/OS, use an approach similar to kLIBC, with 3 lines of z/OS specific code by Igor Todorovski <itodorov@ca.ibm.com>. diff --git a/lib/openat-proc.c b/lib/openat-proc.c index 2a6a85f069..6419a8cf5f 100644 --- a/lib/openat-proc.c +++ b/lib/openat-proc.c @@ -30,9 +30,12 @@ #include <string.h> #include <unistd.h> -#ifdef __KLIBC__ +#ifdef __KLIBC__ /* OS/2 */ # include <InnoTekLIBC/backend.h> #endif +#ifdef __MVS__ /* z/OS */ +# include <termios.h> +#endif #include "intprops.h" @@ -53,7 +56,8 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) return buf; } -#ifndef __KLIBC__ +#if !(defined __KLIBC__ || defined __MVS__) + /* Generic code for Linux, Solaris, and similar platforms. */ # define PROC_SELF_FD_FORMAT "/proc/self/fd/%d/" { enum { @@ -107,14 +111,21 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) dirlen = sprintf (result, PROC_SELF_FD_FORMAT, fd); } } -#else +#else /* (defined __KLIBC__ || defined __MVS__), i.e. OS/2 or z/OS */ /* OS/2 kLIBC provides a function to retrieve a path from a fd. */ { - char dir[_MAX_PATH]; size_t bufsize; +# ifdef __KLIBC__ + char dir[_MAX_PATH]; if (__libc_Back_ioFHToPath (fd, dir, sizeof dir)) return NULL; +# endif +# ifdef __MVS__ + char dir[_XOPEN_PATH_MAX]; + if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) == 0) + __e2a_l (dir, sizeof dir); +# endif dirlen = strlen (dir); bufsize = dirlen + 1 + strlen (file) + 1; /* 1 for '/', 1 for null */ [-- Attachment #2: Type: text/html, Size: 12361 bytes --] ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: Z/OS Enhancement to gnu lib 2023-01-30 17:29 ` Igor Todorovski @ 2023-01-30 22:12 ` Bruno Haible 0 siblings, 0 replies; 4+ messages in thread From: Bruno Haible @ 2023-01-30 22:12 UTC (permalink / raw) To: bug-gnulib, Igor Todorovski; +Cc: Mike Fulton Igor Todorovski wrote: > This was the symptom we experienced with findutils (guarding the close in gnulib fixed it): > find . -name "test" > The output is: > find: '.': EDC5113I Bad file descriptor. > > https://github.com/ZOSOpenTools/findutilsport/issues/4 > > We’ll do some more testing to see if there’s a better solution. I'm sending you a testdir, generated through ./gnulib-tool --create-testdir --dir=/tmp/testdir --single-configure \ areadlinkat areadlinkat-with-size faccessat fchmodat fchownat fdopendir getcwd fstatat linkat mkdirat mkfifoat openat readlinkat renameatu savedir symlinkat unlinkat utimensat As part of your testing, please run it through ./configure && make && make check (Sent via private mail, as it's too large for this mailing list.) > __e2a_l is documented here: https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-e2a-l-convert-characters-from-ebcdic-ascii Well, according to this documentation your patch was buggy: it did not check the return value. Additionally, it ran a conversion on a buffer full of uninitialized data (which is 1. slow and 2. triggers warnings in tools like valgrind). And when w_ioctl failed, your code was accessing and returning unitialized data in dir[]. Ouch ouch ouch. Fixing it like this: 2023-01-30 Bruno Haible <bruno@clisp.org> at-internal: Fix support for z/OS. * lib/openat-proc.c (openat_proc_name) [z/OS]: Proper error handling. Convert only the relevant part of the dir[] buffer. diff --git a/lib/openat-proc.c b/lib/openat-proc.c index 6419a8cf5f..88f70be4f5 100644 --- a/lib/openat-proc.c +++ b/lib/openat-proc.c @@ -123,8 +123,16 @@ openat_proc_name (char buf[OPENAT_BUFFER_SIZE], int fd, char const *file) # endif # ifdef __MVS__ char dir[_XOPEN_PATH_MAX]; - if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) == 0) - __e2a_l (dir, sizeof dir); + /* Documentation: + https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-w-ioctl-w-pioctl-control-devices */ + if (w_ioctl (fd, _IOCC_GPN, sizeof dir, dir) < 0) + return NULL; + /* Documentation: + https://www.ibm.com/docs/en/zos/2.2.0?topic=functions-e2a-l-convert-characters-from-ebcdic-ascii */ + dirlen = __e2a_l (dir, strlen (dir)); + if (dirlen < 0 || dirlen >= sizeof dir) + return NULL; + dir[dirlen] = '\0'; # endif dirlen = strlen (dir); ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-01-30 22:12 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-30 4:09 Z/OS Enhancement to gnu lib Igor Todorovski 2023-01-30 11:48 ` Bruno Haible 2023-01-30 17:29 ` Igor Todorovski 2023-01-30 22:12 ` Bruno Haible
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).