diff mbox series

[2/2] Revert "linux: Move xmknod{at} to compat symbols"

Message ID 20201022190302.2000118-2-adhemerval.zanella@linaro.org
State New
Headers show
Series [1/2] Revert "linux: Move {f}xstat{at} to compat symbols" | expand

Commit Message

Adhemerval Zanella Oct. 22, 2020, 7:03 p.m. UTC
This reverts commit 81b83ff61f95f30ad53d6075247af0ea61a0b16e responsible
to make both __xmknod{at} compatible symbols.

The idea is to not force static libraries built against old glibc
to update against new glibcs (since they reference the the
xmknod{at} symbols).
---
 sysdeps/mach/hurd/xmknod.c         | 11 -----------
 sysdeps/mach/hurd/xmknodat.c       | 11 -----------
 sysdeps/unix/sysv/linux/xmknod.c   |  3 ---
 sysdeps/unix/sysv/linux/xmknodat.c |  2 --
 4 files changed, 27 deletions(-)

Comments

Florian Weimer Oct. 23, 2020, 4:35 p.m. UTC | #1
* Adhemerval Zanella:

> This reverts commit 81b83ff61f95f30ad53d6075247af0ea61a0b16e responsible
> to make both __xmknod{at} compatible symbols.
>
> The idea is to not force static libraries built against old glibc
> to update against new glibcs (since they reference the the
> xmknod{at} symbols).

I think it's not correct to label this as a revert in the Git sense.
At best it's a partial revert.  I tried first to verify this as an
actual revert and was wondering if I got the diffing wrong.

I think you need to remove libc_hidden_def and libc_hidden_proto around
__xmknod, __xmknodat.  “git grep 'libc_hidden.*__xmknod'” should not
return anything.  Maybe that should be a separate patch.

Thanks,
Florian
Adhemerval Zanella Oct. 23, 2020, 4:39 p.m. UTC | #2
On 23/10/2020 13:35, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> This reverts commit 81b83ff61f95f30ad53d6075247af0ea61a0b16e responsible
>> to make both __xmknod{at} compatible symbols.
>>
>> The idea is to not force static libraries built against old glibc
>> to update against new glibcs (since they reference the the
>> xmknod{at} symbols).
> 
> I think it's not correct to label this as a revert in the Git sense.
> At best it's a partial revert.  I tried first to verify this as an
> actual revert and was wondering if I got the diffing wrong.

Ok, I will change to a partial remove.

> 
> I think you need to remove libc_hidden_def and libc_hidden_proto around
> __xmknod, __xmknodat.  “git grep 'libc_hidden.*__xmknod'” should not
> return anything.  Maybe that should be a separate patch.

If I recall correctly they are still required for Hurd, but I double
check.  In fact I think we should remove all the io/*xstat* and
io/*xmknod* since there is no point in providing them as generic
interfaces.
Florian Weimer Oct. 23, 2020, 4:43 p.m. UTC | #3
* Adhemerval Zanella:

> On 23/10/2020 13:35, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> This reverts commit 81b83ff61f95f30ad53d6075247af0ea61a0b16e responsible
>>> to make both __xmknod{at} compatible symbols.
>>>
>>> The idea is to not force static libraries built against old glibc
>>> to update against new glibcs (since they reference the the
>>> xmknod{at} symbols).
>> 
>> I think it's not correct to label this as a revert in the Git sense.
>> At best it's a partial revert.  I tried first to verify this as an
>> actual revert and was wondering if I got the diffing wrong.
>
> Ok, I will change to a partial remove.
>
>> 
>> I think you need to remove libc_hidden_def and libc_hidden_proto around
>> __xmknod, __xmknodat.  “git grep 'libc_hidden.*__xmknod'” should not
>> return anything.  Maybe that should be a separate patch.
>
> If I recall correctly they are still required for Hurd, but I double
> check.  In fact I think we should remove all the io/*xstat* and
> io/*xmknod* since there is no point in providing them as generic
> interfaces.

Then I think you should add the missing libc_hidden_defs instead (to the
Linux implementations).  The present state is inconsistent and
potentially brittle.

Thanks,
Florian
Adhemerval Zanella Oct. 23, 2020, 4:47 p.m. UTC | #4
On 23/10/2020 13:43, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 23/10/2020 13:35, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> This reverts commit 81b83ff61f95f30ad53d6075247af0ea61a0b16e responsible
>>>> to make both __xmknod{at} compatible symbols.
>>>>
>>>> The idea is to not force static libraries built against old glibc
>>>> to update against new glibcs (since they reference the the
>>>> xmknod{at} symbols).
>>>
>>> I think it's not correct to label this as a revert in the Git sense.
>>> At best it's a partial revert.  I tried first to verify this as an
>>> actual revert and was wondering if I got the diffing wrong.
>>
>> Ok, I will change to a partial remove.
>>
>>>
>>> I think you need to remove libc_hidden_def and libc_hidden_proto around
>>> __xmknod, __xmknodat.  “git grep 'libc_hidden.*__xmknod'” should not
>>> return anything.  Maybe that should be a separate patch.
>>
>> If I recall correctly they are still required for Hurd, but I double
>> check.  In fact I think we should remove all the io/*xstat* and
>> io/*xmknod* since there is no point in providing them as generic
>> interfaces.
> 
> Then I think you should add the missing libc_hidden_defs instead (to the
> Linux implementations).  The present state is inconsistent and
> potentially brittle.

Why do we need to add the libc_hidden_defs for Linux? Currently they are not
used internally anymore (both xmknod and xmknodat will call __mknodat).

I think it would be better to check if the hidden_proto/def are stil required
for Hurd and it were to move the libc_hidden_def to a Hurd only header.
Florian Weimer Oct. 23, 2020, 5:18 p.m. UTC | #5
* Adhemerval Zanella:

>>> If I recall correctly they are still required for Hurd, but I double
>>> check.  In fact I think we should remove all the io/*xstat* and
>>> io/*xmknod* since there is no point in providing them as generic
>>> interfaces.
>> 
>> Then I think you should add the missing libc_hidden_defs instead (to the
>> Linux implementations).  The present state is inconsistent and
>> potentially brittle.
>
> Why do we need to add the libc_hidden_defs for Linux? Currently they are not
> used internally anymore (both xmknod and xmknodat will call __mknodat).

I think they always have to come in pairs, conceptually.  Some pairings
work by accident.  I think the aliasing could be implemented in the
other direction, then this use case (public symbol, no internal
references) would break, but the other one (unused export, internal
references) would work.

Thanks,
Florian
Adhemerval Zanella Oct. 23, 2020, 6:56 p.m. UTC | #6
On 23/10/2020 14:18, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>>>> If I recall correctly they are still required for Hurd, but I double
>>>> check.  In fact I think we should remove all the io/*xstat* and
>>>> io/*xmknod* since there is no point in providing them as generic
>>>> interfaces.
>>>
>>> Then I think you should add the missing libc_hidden_defs instead (to the
>>> Linux implementations).  The present state is inconsistent and
>>> potentially brittle.
>>
>> Why do we need to add the libc_hidden_defs for Linux? Currently they are not
>> used internally anymore (both xmknod and xmknodat will call __mknodat).
> 
> I think they always have to come in pairs, conceptually.  Some pairings
> work by accident.  I think the aliasing could be implemented in the
> other direction, then this use case (public symbol, no internal
> references) would break, but the other one (unused export, internal
> references) would work.
For this specific case, the libc_hidden_proto on include/sys/stat.h are
only defined for NO_RTLD_HIDDEN, which is only define for Hurd.  So I
don't think there is the need to add the libc_hidden_def for the Linux
implementation. 

This is also for the xstat symbols.
Florian Weimer Oct. 26, 2020, 12:56 p.m. UTC | #7
* Adhemerval Zanella:

> On 23/10/2020 14:18, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>>>> If I recall correctly they are still required for Hurd, but I double
>>>>> check.  In fact I think we should remove all the io/*xstat* and
>>>>> io/*xmknod* since there is no point in providing them as generic
>>>>> interfaces.
>>>>
>>>> Then I think you should add the missing libc_hidden_defs instead (to the
>>>> Linux implementations).  The present state is inconsistent and
>>>> potentially brittle.
>>>
>>> Why do we need to add the libc_hidden_defs for Linux? Currently they are not
>>> used internally anymore (both xmknod and xmknodat will call __mknodat).
>> 
>> I think they always have to come in pairs, conceptually.  Some pairings
>> work by accident.  I think the aliasing could be implemented in the
>> other direction, then this use case (public symbol, no internal
>> references) would break, but the other one (unused export, internal
>> references) would work.

> For this specific case, the libc_hidden_proto on include/sys/stat.h are
> only defined for NO_RTLD_HIDDEN, which is only define for Hurd.  So I
> don't think there is the need to add the libc_hidden_def for the Linux
> implementation. 
>
> This is also for the xstat symbols.

Hmm.  I really don't like this.  But we can consider it separately from
the (partial) revert.

Thanks,
Florian
Adhemerval Zanella Oct. 26, 2020, 4:24 p.m. UTC | #8
On 26/10/2020 09:56, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> On 23/10/2020 14:18, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>>>> If I recall correctly they are still required for Hurd, but I double
>>>>>> check.  In fact I think we should remove all the io/*xstat* and
>>>>>> io/*xmknod* since there is no point in providing them as generic
>>>>>> interfaces.
>>>>>
>>>>> Then I think you should add the missing libc_hidden_defs instead (to the
>>>>> Linux implementations).  The present state is inconsistent and
>>>>> potentially brittle.
>>>>
>>>> Why do we need to add the libc_hidden_defs for Linux? Currently they are not
>>>> used internally anymore (both xmknod and xmknodat will call __mknodat).
>>>
>>> I think they always have to come in pairs, conceptually.  Some pairings
>>> work by accident.  I think the aliasing could be implemented in the
>>> other direction, then this use case (public symbol, no internal
>>> references) would break, but the other one (unused export, internal
>>> references) would work.
> 
>> For this specific case, the libc_hidden_proto on include/sys/stat.h are
>> only defined for NO_RTLD_HIDDEN, which is only define for Hurd.  So I
>> don't think there is the need to add the libc_hidden_def for the Linux
>> implementation. 
>>
>> This is also for the xstat symbols.
> 
> Hmm.  I really don't like this.  But we can consider it separately from
> the (partial) revert.

Could we track this with a different patch? I would like to close this
and the xstat issue.
Florian Weimer Oct. 26, 2020, 4:30 p.m. UTC | #9
* Adhemerval Zanella:

> On 26/10/2020 09:56, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> On 23/10/2020 14:18, Florian Weimer wrote:
>>>> * Adhemerval Zanella:
>>>>
>>>>>>> If I recall correctly they are still required for Hurd, but I double
>>>>>>> check.  In fact I think we should remove all the io/*xstat* and
>>>>>>> io/*xmknod* since there is no point in providing them as generic
>>>>>>> interfaces.
>>>>>>
>>>>>> Then I think you should add the missing libc_hidden_defs instead (to the
>>>>>> Linux implementations).  The present state is inconsistent and
>>>>>> potentially brittle.
>>>>>
>>>>> Why do we need to add the libc_hidden_defs for Linux? Currently they are not
>>>>> used internally anymore (both xmknod and xmknodat will call __mknodat).
>>>>
>>>> I think they always have to come in pairs, conceptually.  Some pairings
>>>> work by accident.  I think the aliasing could be implemented in the
>>>> other direction, then this use case (public symbol, no internal
>>>> references) would break, but the other one (unused export, internal
>>>> references) would work.
>> 
>>> For this specific case, the libc_hidden_proto on include/sys/stat.h are
>>> only defined for NO_RTLD_HIDDEN, which is only define for Hurd.  So I
>>> don't think there is the need to add the libc_hidden_def for the Linux
>>> implementation. 
>>>
>>> This is also for the xstat symbols.
>> 
>> Hmm.  I really don't like this.  But we can consider it separately from
>> the (partial) revert.
>
> Could we track this with a different patch?

Yes, that's what I meant.

Thanks,
Florian
diff mbox series

Patch

diff --git a/sysdeps/mach/hurd/xmknod.c b/sysdeps/mach/hurd/xmknod.c
index a1b5af086d..fa69bc3bec 100644
--- a/sysdeps/mach/hurd/xmknod.c
+++ b/sysdeps/mach/hurd/xmknod.c
@@ -17,7 +17,6 @@ 
 
 #include <sys/stat.h>
 #include <fcntl.h>
-#include <shlib-compat.h>
 
 /* Create a device file named FILE_NAME, with permission and special bits MODE
    and device number DEV (which can be constructed from major and minor
@@ -28,13 +27,3 @@  __xmknod (int vers, const char *file_name, mode_t mode, dev_t *dev)
   return __xmknodat (vers, AT_FDCWD, file_name, mode, dev);
 }
 libc_hidden_def (__xmknod)
-
-#if SHLIB_COMPAT(libc, GLIBC_2_0, GLIBC_2_33)
-int
-__xmknod_compat (int vers, const char *file_name, mode_t mode, dev_t *dev)
-{
-  return __xmknod (vers, file_name, mode, dev);
-}
-
-compat_symbol (libc, __xmknod_compat, __xmknod, GLIBC_2_0);
-#endif
diff --git a/sysdeps/mach/hurd/xmknodat.c b/sysdeps/mach/hurd/xmknodat.c
index 90724694d5..e46c63036e 100644
--- a/sysdeps/mach/hurd/xmknodat.c
+++ b/sysdeps/mach/hurd/xmknodat.c
@@ -26,7 +26,6 @@ 
 #include <string.h>
 #include <sys/types.h>
 #include <sys/sysmacros.h>
-#include <shlib-compat.h>
 
 /* Create a device file named PATH relative to FD, with permission and
    special bits MODE and device number DEV (which can be constructed
@@ -118,13 +117,3 @@  __xmknodat (int vers, int fd, const char *path, mode_t mode, dev_t *dev)
   return 0;
 }
 libc_hidden_def (__xmknodat)
-
-#if SHLIB_COMPAT(libc, GLIBC_2_4, GLIBC_2_33)
-int
-__xmknodat_compat (int vers, int fd, const char *path, mode_t mode, dev_t *dev)
-{
-  return __xmknodat (vers, fd, path, mode, dev);
-}
-
-compat_symbol (libc, __xmknodat_compat, __xmknodat, GLIBC_2_4);
-#endif
diff --git a/sysdeps/unix/sysv/linux/xmknod.c b/sysdeps/unix/sysv/linux/xmknod.c
index 82bb0bd953..f7eb104423 100644
--- a/sysdeps/unix/sysv/linux/xmknod.c
+++ b/sysdeps/unix/sysv/linux/xmknod.c
@@ -26,7 +26,6 @@ 
    and device number DEV (which can be constructed from major and minor
    device numbers with the `makedev' macro above).  */
 int
-attribute_compat_text_section
 __xmknod (int vers, const char *path, mode_t mode, dev_t *dev)
 {
   if (vers != _MKNOD_VER)
@@ -34,6 +33,4 @@  __xmknod (int vers, const char *path, mode_t mode, dev_t *dev)
 
   return __mknodat (AT_FDCWD, path, mode, *dev);
 }
-
-compat_symbol (libc, __xmknod, __xmknod, GLIBC_2_0);
 #endif
diff --git a/sysdeps/unix/sysv/linux/xmknodat.c b/sysdeps/unix/sysv/linux/xmknodat.c
index fae3fe54d1..b1a43229c8 100644
--- a/sysdeps/unix/sysv/linux/xmknodat.c
+++ b/sysdeps/unix/sysv/linux/xmknodat.c
@@ -31,6 +31,4 @@  __xmknodat (int vers, int fd, const char *file, mode_t mode, dev_t *dev)
 
   return __mknodat (fd, file, mode, *dev);
 }
-
-compat_symbol (libc, __xmknodat, __xmknodat, GLIBC_2_4);
 #endif