| Message ID | 20171030132857.697B840164619@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Assume that _DIRENT_HAVE_D_TYPE is always defined. | expand |
On 10/30/2017 06:28 AM, Florian Weimer wrote: > References remain in io/fts.c, io/ftw.c, posix/glob.c. These files > are (potentially) externally shared. > > 2017-10-30 Florian Weimer <fweimer@redhat.com> > > * elf/ldconfig.c (search_dir): Assume that _DIRENT_HAVE_D_TYPE is > always defined. > * io/tst-mkdirat.c (do_test): Likewise. > * io/tst-mkfifoat.c (do_test): Likewise. > * io/tst-mknodat.c (do_test): Likewise. > * locale/programs/charmap-dir.c (charmap_readdir): Likewise. > * locale/programs/locale.c (select_dirs): Likewise. > * locale/programs/locarchive.c (add_locales_to_archive): Likewise. > * posix/bug-glob2.c (my_readdir): Likewise. > * posix/tst-dir.c (main): Likewise. > * posix/tst-glob_lstat_compat.c (my_readdir): Likewise. > * posix/tst-gnuglob-skeleton.c (my_readdir): Likewise. Everything up to here is OK, and can be committed right away. > * sysdeps/posix/getcwd.c (__getcwd): Likewise. Our shared source file list says this comes from 'gnulib': https://sourceware.org/glibc/wiki/SharedSourceFiles Are we able to harmonize this with gnulib?
On 10/30/2017 03:40 PM, Carlos O'Donell wrote: > On 10/30/2017 06:28 AM, Florian Weimer wrote: >> References remain in io/fts.c, io/ftw.c, posix/glob.c. These files >> are (potentially) externally shared. >> >> 2017-10-30 Florian Weimer <fweimer@redhat.com> >> >> * elf/ldconfig.c (search_dir): Assume that _DIRENT_HAVE_D_TYPE is >> always defined. >> * io/tst-mkdirat.c (do_test): Likewise. >> * io/tst-mkfifoat.c (do_test): Likewise. >> * io/tst-mknodat.c (do_test): Likewise. >> * locale/programs/charmap-dir.c (charmap_readdir): Likewise. >> * locale/programs/locale.c (select_dirs): Likewise. >> * locale/programs/locarchive.c (add_locales_to_archive): Likewise. >> * posix/bug-glob2.c (my_readdir): Likewise. >> * posix/tst-dir.c (main): Likewise. >> * posix/tst-glob_lstat_compat.c (my_readdir): Likewise. >> * posix/tst-gnuglob-skeleton.c (my_readdir): Likewise. > > Everything up to here is OK, and can be committed right away. Thanks. >> * sysdeps/posix/getcwd.c (__getcwd): Likewise. > > Our shared source file list says this comes from 'gnulib': > https://sourceware.org/glibc/wiki/SharedSourceFiles > > Are we able to harmonize this with gnulib? Not sure if this is a good idea. We use the generic code as a building block for the Linux implementation, and I doubt the gnulib implementation will support such abuse. Thanks, Florian
On 10/30/2017 07:46 AM, Florian Weimer wrote: >> Our shared source file list says this comes from 'gnulib': >> https://sourceware.org/glibc/wiki/SharedSourceFiles >> >> Are we able to harmonize this with gnulib? > > Not sure if this is a good idea. We use the generic code as a > building block for the Linux implementation, and I doubt the gnulib > implementation will support such abuse. We have two high-level options: (a) Approach gnulib with a patch that removes the conditional on _DIRENT_HAVE_D_TYPE, and make the case. (b) Stop sharing this source file with gnulib, update the SharedSourceFiles not to list the file. Provide a comment in the file stating all of the historical details and why we are not sharing anymore. You could do (a) today, there is consensus for that. I think (b) needs a distinct discussion in a new thread to gather input. I leave it up to you to go down either path as you think appropriate. I'm on the fence. The getcwd.c implementation isn't so complex that we can't just have our own copy, but at the same time, any meaningful deviation from gnulib seems spurious. Therefore I think there is slightly more inertia for trying to stay harmonized with gnulib.
On 10/30/2017 04:10 PM, Carlos O'Donell wrote: > On 10/30/2017 07:46 AM, Florian Weimer wrote: >>> Our shared source file list says this comes from 'gnulib': >>> https://sourceware.org/glibc/wiki/SharedSourceFiles >>> >>> Are we able to harmonize this with gnulib? >> >> Not sure if this is a good idea. We use the generic code as a >> building block for the Linux implementation, and I doubt the gnulib >> implementation will support such abuse. > > We have two high-level options: > > (a) Approach gnulib with a patch that removes the conditional on > _DIRENT_HAVE_D_TYPE, and make the case. Solaris doesn't have d_type, I think, so that's not really an option for them. > (b) Stop sharing this source file with gnulib, update the SharedSourceFiles > not to list the file. Provide a comment in the file stating all of the > historical details and why we are not sharing anymore. If we do that, we should remove the other untested clutter as well, and the d_type cleanup would just be a tiny part of that. I expect that we can't import gnulib getcwd because the code calls system getcwd under certain circumstances, and that's obviously a bad idea when we use it to implement system getcwd. We could compile it with the appropriate preprocessor flags so that this does not happen, but our own goals and gnulib's clearly have diverged by this point. Thanks, Florian
On 10/30/2017 08:18 AM, Florian Weimer wrote: >> (b) Stop sharing this source file with gnulib, update the >> SharedSourceFiles not to list the file. Provide a comment in the >> file stating all of the historical details and why we are not >> sharing anymore. > > If we do that, we should remove the other untested clutter as well, > and the d_type cleanup would just be a tiny part of that. > > I expect that we can't import gnulib getcwd because the code calls > system getcwd under certain circumstances, and that's obviously a bad > idea when we use it to implement system getcwd. We could compile it > with the appropriate preprocessor flags so that this does not happen, > but our own goals and gnulib's clearly have diverged by this point. That sounds like good rationale for (b) then. Please start a new thread if you haven't.
diff --git a/elf/ldconfig.c b/elf/ldconfig.c index 99caf9e9bb..89042351f8 100644 --- a/elf/ldconfig.c +++ b/elf/ldconfig.c @@ -710,25 +710,20 @@ search_dir (const struct dir_entry *entry) while ((direntry = readdir64 (dir)) != NULL) { int flag; -#ifdef _DIRENT_HAVE_D_TYPE /* We only look at links and regular files. */ if (direntry->d_type != DT_UNKNOWN && direntry->d_type != DT_LNK && direntry->d_type != DT_REG && direntry->d_type != DT_DIR) continue; -#endif /* _DIRENT_HAVE_D_TYPE */ /* Does this file look like a shared library or is it a hwcap subdirectory? The dynamic linker is also considered as shared library. */ if (((strncmp (direntry->d_name, "lib", 3) != 0 && strncmp (direntry->d_name, "ld-", 3) != 0) || strstr (direntry->d_name, ".so") == NULL) - && ( -#ifdef _DIRENT_HAVE_D_TYPE - direntry->d_type == DT_REG || -#endif - !is_hwcap_platform (direntry->d_name))) + && (direntry->d_type == DT_REG + || !is_hwcap_platform (direntry->d_name))) continue; size_t len = strlen (direntry->d_name); @@ -765,12 +760,10 @@ search_dir (const struct dir_entry *entry) } struct stat64 lstat_buf; -#ifdef _DIRENT_HAVE_D_TYPE /* We optimize and try to do the lstat call only if needed. */ if (direntry->d_type != DT_UNKNOWN) lstat_buf.st_mode = DTTOIF (direntry->d_type); else -#endif if (__glibc_unlikely (lstat64 (real_file_name, &lstat_buf))) { error (0, errno, _("Cannot lstat %s"), file_name); @@ -825,9 +818,6 @@ search_dir (const struct dir_entry *entry) new_entry->path = xstrdup (file_name); new_entry->flag = entry->flag; new_entry->next = NULL; -#ifdef _DIRENT_HAVE_D_TYPE - /* We have filled in lstat only #ifndef - _DIRENT_HAVE_D_TYPE. Fill it in if needed. */ if (!is_link && direntry->d_type != DT_UNKNOWN && __builtin_expect (lstat64 (real_file_name, &lstat_buf), 0)) @@ -837,7 +827,6 @@ search_dir (const struct dir_entry *entry) free (new_entry); continue; } -#endif new_entry->ino = lstat_buf.st_ino; new_entry->dev = lstat_buf.st_dev; add_single_dir (new_entry, 0); @@ -860,7 +849,6 @@ search_dir (const struct dir_entry *entry) else real_name = real_file_name; -#ifdef _DIRENT_HAVE_D_TYPE /* Call lstat64 if not done yet. */ if (!is_link && direntry->d_type != DT_UNKNOWN @@ -869,7 +857,6 @@ search_dir (const struct dir_entry *entry) error (0, errno, _("Cannot lstat %s"), file_name); continue; } -#endif /* First search whether the auxiliary cache contains this library already and it's not changed. */ diff --git a/io/tst-mkdirat.c b/io/tst-mkdirat.c index a960c6651d..605e51ef1e 100644 --- a/io/tst-mkdirat.c +++ b/io/tst-mkdirat.c @@ -131,13 +131,11 @@ do_test (void) if (strcmp (d->d_name, "some-dir") == 0) { has_some_dir = true; -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_DIR) { puts ("d_type for some-dir wrong"); return 1; } -#endif } else if (strcmp (d->d_name, ".") != 0 && strcmp (d->d_name, "..") != 0) { diff --git a/io/tst-mkfifoat.c b/io/tst-mkfifoat.c index d87b587384..c9239dcde8 100644 --- a/io/tst-mkfifoat.c +++ b/io/tst-mkfifoat.c @@ -131,13 +131,11 @@ do_test (void) if (strcmp (d->d_name, "some-fifo") == 0) { has_some_fifo = true; -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_FIFO) { puts ("d_type for some-fifo wrong"); return 1; } -#endif } else if (strcmp (d->d_name, ".") != 0 && strcmp (d->d_name, "..") != 0) { diff --git a/io/tst-mknodat.c b/io/tst-mknodat.c index 9d58fdbe3a..88a98cab79 100644 --- a/io/tst-mknodat.c +++ b/io/tst-mknodat.c @@ -131,13 +131,11 @@ do_test (void) if (strcmp (d->d_name, "some-fifo") == 0) { has_some_fifo = true; -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_FIFO) { puts ("d_type for some-fifo wrong"); return 1; } -#endif } else if (strcmp (d->d_name, ".") != 0 && strcmp (d->d_name, "..") != 0) { diff --git a/locale/programs/charmap-dir.c b/locale/programs/charmap-dir.c index a9212b72fb..42639fbc1b 100644 --- a/locale/programs/charmap-dir.c +++ b/locale/programs/charmap-dir.c @@ -115,11 +115,9 @@ charmap_readdir (CHARMAP_DIR *cdir) stpcpy (stpcpy (cdir->pathname, cdir->directory), dirent->d_name); filename = cdir->pathname + cdir->directory_len; -#ifdef _DIRENT_HAVE_D_TYPE if (dirent->d_type != DT_UNKNOWN && dirent->d_type != DT_LNK) mode = DTTOIF (dirent->d_type); else -#endif { struct stat64 statbuf; diff --git a/locale/programs/locale.c b/locale/programs/locale.c index 939214dbd0..de2a30551c 100644 --- a/locale/programs/locale.c +++ b/locale/programs/locale.c @@ -316,11 +316,9 @@ select_dirs (const struct dirent *dirent) { mode_t mode = 0; -#ifdef _DIRENT_HAVE_D_TYPE if (dirent->d_type != DT_UNKNOWN && dirent->d_type != DT_LNK) mode = DTTOIF (dirent->d_type); else -#endif { struct stat64 st; char buf[sizeof (COMPLOCALEDIR) diff --git a/locale/programs/locarchive.c b/locale/programs/locarchive.c index 633c59b5be..50e975df20 100644 --- a/locale/programs/locarchive.c +++ b/locale/programs/locarchive.c @@ -1385,17 +1385,13 @@ add_locales_to_archive (size_t nlist, char *list[], bool replace) a directory we have to look at a file with the prefix "SYS_". Otherwise we have found what we are looking for. */ -#ifdef _DIRENT_HAVE_D_TYPE d_type = d->d_type; if (d_type != DT_REG) -#endif { char fullname[fnamelen + 2 * strlen (d->d_name) + 7]; -#ifdef _DIRENT_HAVE_D_TYPE if (d_type == DT_UNKNOWN) -#endif { strcpy (stpcpy (stpcpy (fullname, fname), "/"), d->d_name); diff --git a/posix/bug-glob2.c b/posix/bug-glob2.c index 592d957a75..c9ed76e134 100644 --- a/posix/bug-glob2.c +++ b/posix/bug-glob2.c @@ -207,21 +207,13 @@ my_readdir (void *gdir) dir->d.d_ino = 1; /* glob should not skip this entry. */ -#ifdef _DIRENT_HAVE_D_TYPE dir->d.d_type = filesystem[dir->idx].type; -#endif strcpy (dir->d.d_name, filesystem[dir->idx].name); -#ifdef _DIRENT_HAVE_D_TYPE PRINTF ("my_readdir ({ level: %d, idx: %ld }) = { d_ino: %ld, d_type: %d, d_name: \"%s\" }\n", dir->level, (long int) dir->idx, dir->d.d_ino, dir->d.d_type, dir->d.d_name); -#else - PRINTF ("my_readdir ({ level: %d, idx: %ld }) = { d_ino: %ld, d_name: \"%s\" }\n", - dir->level, (long int) dir->idx, dir->d.d_ino, - dir->d.d_name); -#endif ++dir->idx; diff --git a/posix/tst-dir.c b/posix/tst-dir.c index fee79b32a0..52ff36fb6e 100644 --- a/posix/tst-dir.c +++ b/posix/tst-dir.c @@ -149,10 +149,8 @@ main (int argc, char *argv[]) while ((d = readdir64 (dir1)) != NULL) { -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_REG) continue; -#endif if (d->d_ino == st2.st_ino) { @@ -234,10 +232,8 @@ main (int argc, char *argv[]) while ((d = readdir64 (dir2)) != NULL) { -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_DIR) continue; -#endif if (d->d_ino == st2.st_ino) { @@ -327,10 +323,8 @@ main (int argc, char *argv[]) rewinddir (dir1); while (readdir64_r (dir1, &direntbuf.d, &d) == 0 && d != NULL) { -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_DIR) continue; -#endif if (d->d_ino == st1.st_ino) { @@ -457,13 +451,11 @@ main (int argc, char *argv[]) || strcmp (d->d_name, "..") == 0 || strcmp (d->d_name, "another-dir") == 0) { -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_DIR) { printf ("d_type for \"%s\" is wrong\n", d->d_name); result = 1; } -#endif if (stat64 (d->d_name, &st3) < 0) { printf ("cannot stat \"%s\" is wrong\n", d->d_name); @@ -477,13 +469,11 @@ main (int argc, char *argv[]) } else if (strcmp (d->d_name, "and-a-file") == 0) { -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_UNKNOWN && d->d_type != DT_REG) { printf ("d_type for \"%s\" is wrong\n", d->d_name); result = 1; } -#endif if (stat64 (d->d_name, &st3) < 0) { printf ("cannot stat \"%s\" is wrong\n", d->d_name); diff --git a/posix/tst-glob_lstat_compat.c b/posix/tst-glob_lstat_compat.c index ccfda4bb74..6b77909104 100644 --- a/posix/tst-glob_lstat_compat.c +++ b/posix/tst-glob_lstat_compat.c @@ -166,9 +166,7 @@ my_readdir (void *gdir) dir->d.d_ino = 1; /* glob should not skip this entry. */ -#ifdef _DIRENT_HAVE_D_TYPE dir->d.d_type = filesystem[dir->idx].type; -#endif strcpy (dir->d.d_name, filesystem[dir->idx].name); diff --git a/posix/tst-gnuglob-skeleton.c b/posix/tst-gnuglob-skeleton.c index 9276297c7c..ba6812bf59 100644 --- a/posix/tst-gnuglob-skeleton.c +++ b/posix/tst-gnuglob-skeleton.c @@ -221,27 +221,16 @@ my_readdir (void *gdir) dir->d.d_ino = 1; /* glob should not skip this entry. */ -#ifdef _DIRENT_HAVE_D_TYPE dir->d.d_type = filesystem[dir->idx].type; -#endif strcpy (dir->d.d_name, filesystem[dir->idx].name); -#ifdef _DIRENT_HAVE_D_TYPE if (test_verbose > 0) printf ("info: my_readdir ({ level: %d, idx: %ld })" " = { d_ino: %lld, d_type: %d, d_name: \"%s\" }\n", dir->level, (long int) dir->idx, (long long) dir->d.d_ino, dir->d.d_type, dir->d.d_name); -#else - if (test_verbose > 0) - printf ("info: my_readdir ({ level: %d, idx: %ld })" - " = { d_ino: %lld, d_name: \"%s\" }\n", - dir->level, (long int) dir->idx, - (long long) dir->d.d_ino, - dir->d.d_name); -#endif ++dir->idx; diff --git a/sysdeps/posix/getcwd.c b/sysdeps/posix/getcwd.c index 702a052306..a0548951f4 100644 --- a/sysdeps/posix/getcwd.c +++ b/sysdeps/posix/getcwd.c @@ -397,10 +397,8 @@ __getcwd (char *buf, size_t size) goto lose; } -#ifdef _DIRENT_HAVE_D_TYPE if (d->d_type != DT_DIR && d->d_type != DT_UNKNOWN) continue; -#endif if (d->d_name[0] == '.' && (d->d_name[1] == '\0' || (d->d_name[1] == '.' && d->d_name[2] == '\0')))