[1/2] define daddr_t type

Submitted by Petr Vorel on June 7, 2019, 5:14 a.m.

Details

Message ID 20190607051444.20316-1-petr.vorel@gmail.com
State New
Series "Series without cover letter"
Headers show

Commit Message

Petr Vorel June 7, 2019, 5:14 a.m.
According to kernel sources only mips (and sparc which we don't support)
defines daddr_t as long, other define int.

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 arch/mips/bits/alltypes.h.in    | 2 ++
 arch/mipsn32/bits/alltypes.h.in | 2 ++
 include/alltypes.h.in           | 1 +
 include/sys/types.h             | 1 +
 4 files changed, 6 insertions(+)

Patch hide | download patch | download mbox

diff --git a/arch/mips/bits/alltypes.h.in b/arch/mips/bits/alltypes.h.in
index 66ca18ad..bd062a85 100644
--- a/arch/mips/bits/alltypes.h.in
+++ b/arch/mips/bits/alltypes.h.in
@@ -17,6 +17,8 @@  TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
 TYPEDEF long time_t;
 TYPEDEF long suseconds_t;
 
+TYPEDEF long daddr_t;
+
 TYPEDEF struct { union { int __i[9]; volatile int __vi[9]; unsigned __s[9]; } __u; } pthread_attr_t;
 TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
 TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
diff --git a/arch/mipsn32/bits/alltypes.h.in b/arch/mipsn32/bits/alltypes.h.in
index 66ca18ad..bd062a85 100644
--- a/arch/mipsn32/bits/alltypes.h.in
+++ b/arch/mipsn32/bits/alltypes.h.in
@@ -17,6 +17,8 @@  TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
 TYPEDEF long time_t;
 TYPEDEF long suseconds_t;
 
+TYPEDEF long daddr_t;
+
 TYPEDEF struct { union { int __i[9]; volatile int __vi[9]; unsigned __s[9]; } __u; } pthread_attr_t;
 TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
 TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
diff --git a/include/alltypes.h.in b/include/alltypes.h.in
index 4cc879b1..6ef6ebd4 100644
--- a/include/alltypes.h.in
+++ b/include/alltypes.h.in
@@ -18,6 +18,7 @@  TYPEDEF unsigned _Int64 uint64_t;
 TYPEDEF unsigned _Int64 u_int64_t;
 TYPEDEF unsigned _Int64 uintmax_t;
 
+TYPEDEF int daddr_t;
 TYPEDEF unsigned mode_t;
 TYPEDEF unsigned _Reg nlink_t;
 TYPEDEF _Int64 off_t;
diff --git a/include/sys/types.h b/include/sys/types.h
index 75e489c5..c50d21c9 100644
--- a/include/sys/types.h
+++ b/include/sys/types.h
@@ -29,6 +29,7 @@  extern "C" {
 #define __NEED_clock_t
 #define __NEED_suseconds_t
 #define __NEED_blksize_t
+#define __NEED_daddr_t
 
 #define __NEED_pthread_t
 #define __NEED_pthread_attr_t

Comments

Rich Felker June 7, 2019, 5:28 a.m.
On Fri, Jun 07, 2019 at 07:14:43AM +0200, Petr Vorel wrote:
> According to kernel sources only mips (and sparc which we don't support)
> defines daddr_t as long, other define int.
> 
> Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
> ---
>  arch/mips/bits/alltypes.h.in    | 2 ++
>  arch/mipsn32/bits/alltypes.h.in | 2 ++
>  include/alltypes.h.in           | 1 +
>  include/sys/types.h             | 1 +
>  4 files changed, 6 insertions(+)
> 
> diff --git a/arch/mips/bits/alltypes.h.in b/arch/mips/bits/alltypes.h.in
> index 66ca18ad..bd062a85 100644
> --- a/arch/mips/bits/alltypes.h.in
> +++ b/arch/mips/bits/alltypes.h.in
> @@ -17,6 +17,8 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
>  TYPEDEF long time_t;
>  TYPEDEF long suseconds_t;
>  
> +TYPEDEF long daddr_t;
> +
>  TYPEDEF struct { union { int __i[9]; volatile int __vi[9]; unsigned __s[9]; } __u; } pthread_attr_t;
>  TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
>  TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
> diff --git a/arch/mipsn32/bits/alltypes.h.in b/arch/mipsn32/bits/alltypes.h.in
> index 66ca18ad..bd062a85 100644
> --- a/arch/mipsn32/bits/alltypes.h.in
> +++ b/arch/mipsn32/bits/alltypes.h.in
> @@ -17,6 +17,8 @@ TYPEDEF struct { long long __ll; long double __ld; } max_align_t;
>  TYPEDEF long time_t;
>  TYPEDEF long suseconds_t;
>  
> +TYPEDEF long daddr_t;
> +
>  TYPEDEF struct { union { int __i[9]; volatile int __vi[9]; unsigned __s[9]; } __u; } pthread_attr_t;
>  TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } pthread_mutex_t;
>  TYPEDEF struct { union { int __i[6]; volatile int __vi[6]; volatile void *volatile __p[6]; } __u; } mtx_t;
> diff --git a/include/alltypes.h.in b/include/alltypes.h.in
> index 4cc879b1..6ef6ebd4 100644
> --- a/include/alltypes.h.in
> +++ b/include/alltypes.h.in
> @@ -18,6 +18,7 @@ TYPEDEF unsigned _Int64 uint64_t;
>  TYPEDEF unsigned _Int64 u_int64_t;
>  TYPEDEF unsigned _Int64 uintmax_t;
>  
> +TYPEDEF int daddr_t;
>  TYPEDEF unsigned mode_t;
>  TYPEDEF unsigned _Reg nlink_t;
>  TYPEDEF _Int64 off_t;
> diff --git a/include/sys/types.h b/include/sys/types.h
> index 75e489c5..c50d21c9 100644
> --- a/include/sys/types.h
> +++ b/include/sys/types.h
> @@ -29,6 +29,7 @@ extern "C" {
>  #define __NEED_clock_t
>  #define __NEED_suseconds_t
>  #define __NEED_blksize_t
> +#define __NEED_daddr_t
>  
>  #define __NEED_pthread_t
>  #define __NEED_pthread_attr_t
> -- 
> 2.21.0

daddr_t is not a standard type, so can't be exposed by default here
(aside from the dubious "*_t is always reserved" rule), and it's only
proposed to be used in one header, so it doesn't belong in alltypes.h
either.

Rich
Petr Vorel June 7, 2019, 5:53 a.m.
Hi Rich,

...
> > +++ b/include/sys/types.h
> > @@ -29,6 +29,7 @@ extern "C" {
> >  #define __NEED_clock_t
> >  #define __NEED_suseconds_t
> >  #define __NEED_blksize_t
> > +#define __NEED_daddr_t
...

> daddr_t is not a standard type, so can't be exposed by default here
> (aside from the dubious "*_t is always reserved" rule), and it's only
So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?

> proposed to be used in one header, so it doesn't belong in alltypes.h
> either.
Where should it be then? Shell I create bits/types.h for it?
The goal is to be loadable from <sys/types.h>

> Rich

Kind regards,
Petr
Rich Felker June 7, 2019, 5:58 a.m.
On Fri, Jun 07, 2019 at 07:53:29AM +0200, Petr Vorel wrote:
> Hi Rich,
> 
> ....
> > > +++ b/include/sys/types.h
> > > @@ -29,6 +29,7 @@ extern "C" {
> > >  #define __NEED_clock_t
> > >  #define __NEED_suseconds_t
> > >  #define __NEED_blksize_t
> > > +#define __NEED_daddr_t
> ....
> 
> > daddr_t is not a standard type, so can't be exposed by default here
> > (aside from the dubious "*_t is always reserved" rule), and it's only
> So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?
> 
> > proposed to be used in one header, so it doesn't belong in alltypes.h
> > either.
> Where should it be then? Shell I create bits/types.h for it?
> The goal is to be loadable from <sys/types.h>

Why? It's not a reasonable type for any application to use -- we've
never gotten a report that something failed to build because of its
absence, and even if we did, it would almost surely be a case of "fix
the application". It looks like the only reason you wanted it was to
fix the type of a field in an mtio structure, and in that case the
type would just need to be defined in mtio.h.

Rich
Petr Vorel June 7, 2019, 6:06 a.m.
Hi Rich,

> > ....
> > > > +++ b/include/sys/types.h
> > > > @@ -29,6 +29,7 @@ extern "C" {
> > > >  #define __NEED_clock_t
> > > >  #define __NEED_suseconds_t
> > > >  #define __NEED_blksize_t
> > > > +#define __NEED_daddr_t
> > ....

> > > daddr_t is not a standard type, so can't be exposed by default here
> > > (aside from the dubious "*_t is always reserved" rule), and it's only
> > So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?

> > > proposed to be used in one header, so it doesn't belong in alltypes.h
> > > either.
> > Where should it be then? Shell I create bits/types.h for it?
> > The goal is to be loadable from <sys/types.h>

> Why? It's not a reasonable type for any application to use -- we've
> never gotten a report that something failed to build because of its
> absence, and even if we did, it would almost surely be a case of "fix
> the application". It looks like the only reason you wanted it was to
> fix the type of a field in an mtio structure, and in that case the
> type would just need to be defined in mtio.h.

I need it for LTP [1]. It's actually workaround for missing struct ustat [2].
If it's really useless to have it in musl, I'll use in LTP __kernel_daddr_t from <linux/types.h>.

[1] https://patchwork.ozlabs.org/patch/1102380/
[2] https://github.com/linux-test-project/ltp/blob/master/include/lapi/ustat.h

> Rich


Kind regards,
Petr
Rich Felker June 7, 2019, 6:18 a.m.
On Fri, Jun 07, 2019 at 08:06:20AM +0200, Petr Vorel wrote:
> Hi Rich,
> 
> > > ....
> > > > > +++ b/include/sys/types.h
> > > > > @@ -29,6 +29,7 @@ extern "C" {
> > > > >  #define __NEED_clock_t
> > > > >  #define __NEED_suseconds_t
> > > > >  #define __NEED_blksize_t
> > > > > +#define __NEED_daddr_t
> > > ....
> 
> > > > daddr_t is not a standard type, so can't be exposed by default here
> > > > (aside from the dubious "*_t is always reserved" rule), and it's only
> > > So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?
> 
> > > > proposed to be used in one header, so it doesn't belong in alltypes.h
> > > > either.
> > > Where should it be then? Shell I create bits/types.h for it?
> > > The goal is to be loadable from <sys/types.h>
> 
> > Why? It's not a reasonable type for any application to use -- we've
> > never gotten a report that something failed to build because of its
> > absence, and even if we did, it would almost surely be a case of "fix
> > the application". It looks like the only reason you wanted it was to
> > fix the type of a field in an mtio structure, and in that case the
> > type would just need to be defined in mtio.h.
> 
> I need it for LTP [1]. It's actually workaround for missing struct ustat [2].
> If it's really useless to have it in musl, I'll use in LTP __kernel_daddr_t from <linux/types.h>.
> 
> [1] https://patchwork.ozlabs.org/patch/1102380/
> [2] https://github.com/linux-test-project/ltp/blob/master/include/lapi/ustat.h

Per http://man7.org/linux/man-pages/man2/ustat.2.html:

    ustat() is deprecated and has been provided only for
    compatibility. All new programs should use statfs(2) instead.

AFAIK we've never hit anything that needed/wanted ustat, and the
interface has fundamental limitations that make it mostly (entirely?)
non-useful. I'm not sure why LTP is testing it...

Rich
Petr Vorel June 7, 2019, 6:28 a.m.
Hi Rich,

...
> Per http://man7.org/linux/man-pages/man2/ustat.2.html:

>     ustat() is deprecated and has been provided only for
>     compatibility. All new programs should use statfs(2) instead.

> AFAIK we've never hit anything that needed/wanted ustat, and the
> interface has fundamental limitations that make it mostly (entirely?)
> non-useful. I'm not sure why LTP is testing it...

Thanks for info. I'll read man page properly next time :).

LTP tests what is in glibc (will test ustat() until it's removed from glibc).
I guess fix for mtio.h is still valid, I'll send a patch.

> Rich

Kind regards,
Petr
Florian Weimer June 7, 2019, 7:22 a.m.
* Petr Vorel:

> LTP tests what is in glibc (will test ustat() until it's removed from glibc).
> I guess fix for mtio.h is still valid, I'll send a patch.

ustat was removed from glibc 2.28.  Therefore, it's missing from the
csky port, which was added in glibc 2.29.

Thanks,
Florian
Petr Vorel June 7, 2019, 7:48 a.m.
Hi Florian,

> * Petr Vorel:

> > LTP tests what is in glibc (will test ustat() until it's removed from glibc).
> > I guess fix for mtio.h is still valid, I'll send a patch.

> ustat was removed from glibc 2.28.  Therefore, it's missing from the
> csky port, which was added in glibc 2.29.
Thanks! Another reason to add autotools check to LTP.

> Thanks,
> Florian

Kind regards,
Petr
Petr Vorel June 7, 2019, 9:21 a.m.
Hi Florian,

> > > LTP tests what is in glibc (will test ustat() until it's removed from glibc).
> > > I guess fix for mtio.h is still valid, I'll send a patch.

> > ustat was removed from glibc 2.28.  Therefore, it's missing from the
> > csky port, which was added in glibc 2.29.
> Thanks! Another reason to add autotools check to LTP.
Actually LTP tests __NR_ustat syscall. But still needs the structure.
But in that case it makes sense to use __kernel_daddr_t (which was internally
used by other libc implementations anyway.

Kind regards,
Petr
Leah Neukirchen June 7, 2019, 12:52 p.m.
Rich Felker <dalias@libc.org> writes:

> On Fri, Jun 07, 2019 at 07:53:29AM +0200, Petr Vorel wrote:
>> Hi Rich,
>> 
>> ....
>> > > +++ b/include/sys/types.h
>> > > @@ -29,6 +29,7 @@ extern "C" {
>> > >  #define __NEED_clock_t
>> > >  #define __NEED_suseconds_t
>> > >  #define __NEED_blksize_t
>> > > +#define __NEED_daddr_t
>> ....
>> 
>> > daddr_t is not a standard type, so can't be exposed by default here
>> > (aside from the dubious "*_t is always reserved" rule), and it's only
>> So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?
>> 
>> > proposed to be used in one header, so it doesn't belong in alltypes.h
>> > either.
>> Where should it be then? Shell I create bits/types.h for it?
>> The goal is to be loadable from <sys/types.h>
>
> Why? It's not a reasonable type for any application to use -- we've
> never gotten a report that something failed to build because of its
> absence, and even if we did, it would almost surely be a case of "fix
> the application".

FWIW, Void patches four packages that use daddr_t:

- gpart
- kpartx
- libtirpc
- sleuthkit

Additionally, five packages use caddr_t:

- ifenslave
- lwipv6
- macchanger
- sbcl
- virtuoso
Rich Felker June 7, 2019, 3:25 p.m.
On Fri, Jun 07, 2019 at 02:52:07PM +0200, Leah Neukirchen wrote:
> Rich Felker <dalias@libc.org> writes:
> 
> > On Fri, Jun 07, 2019 at 07:53:29AM +0200, Petr Vorel wrote:
> >> Hi Rich,
> >> 
> >> ....
> >> > > +++ b/include/sys/types.h
> >> > > @@ -29,6 +29,7 @@ extern "C" {
> >> > >  #define __NEED_clock_t
> >> > >  #define __NEED_suseconds_t
> >> > >  #define __NEED_blksize_t
> >> > > +#define __NEED_daddr_t
> >> ....
> >> 
> >> > daddr_t is not a standard type, so can't be exposed by default here
> >> > (aside from the dubious "*_t is always reserved" rule), and it's only
> >> So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?
> >> 
> >> > proposed to be used in one header, so it doesn't belong in alltypes.h
> >> > either.
> >> Where should it be then? Shell I create bits/types.h for it?
> >> The goal is to be loadable from <sys/types.h>
> >
> > Why? It's not a reasonable type for any application to use -- we've
> > never gotten a report that something failed to build because of its
> > absence, and even if we did, it would almost surely be a case of "fix
> > the application".
> 
> FWIW, Void patches four packages that use daddr_t:
> 
> - gpart
> - kpartx
> - libtirpc
> - sleuthkit
> 
> Additionally, five packages use caddr_t:
> 
> - ifenslave
> - lwipv6
> - macchanger
> - sbcl
> - virtuoso

Thanks. Can you tell if they have any legitimate motivation for doing
so? What even is the conceptual definition of daddr_t? AIUI, caddr_t
was historically something like void * or uintptr_t, a type for
abstract addresses that might not actually point to objects, that
mainly existed because void * didn't yet exist, but I have no idea
what daddr_t is supposed to be.

Rich
Petr Vorel June 7, 2019, 8:57 p.m.
> Rich Felker <dalias@libc.org> writes:

> > On Fri, Jun 07, 2019 at 07:53:29AM +0200, Petr Vorel wrote:
> >> Hi Rich,

> >> ....
> >> > > +++ b/include/sys/types.h
> >> > > @@ -29,6 +29,7 @@ extern "C" {
> >> > >  #define __NEED_clock_t
> >> > >  #define __NEED_suseconds_t
> >> > >  #define __NEED_blksize_t
> >> > > +#define __NEED_daddr_t
> >> ....

> >> > daddr_t is not a standard type, so can't be exposed by default here
> >> > (aside from the dubious "*_t is always reserved" rule), and it's only
> >> So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?

> >> > proposed to be used in one header, so it doesn't belong in alltypes.h
> >> > either.
> >> Where should it be then? Shell I create bits/types.h for it?
> >> The goal is to be loadable from <sys/types.h>

> > Why? It's not a reasonable type for any application to use -- we've
> > never gotten a report that something failed to build because of its
> > absence, and even if we did, it would almost surely be a case of "fix
> > the application".

> FWIW, Void patches four packages that use daddr_t:

> - gpart
UPSTREAM GIT: some solaris related feature (compiled for linux) [1]
struct solaris_x86_slice {
	ushort	s_tag;			/* ID tag of partition */
	ushort	s_flag;			/* permision flags */
	daddr_t s_start;		/* start sector no of partition */
	long	s_size;			/* # of blocks in partition */
};
Void: use int [2].

> - kpartx
UPSTREAM GIT: the same thing, but daddr_t out, they decided to use long [3]

//typedef int daddr_t;		/* or long - check */

struct solaris_x86_slice {
	unsigned short	s_tag;		/* ID tag of partition */
	unsigned short	s_flag;		/* permission flags */
	long		s_start;	/* start sector no of partition */
	long		s_size;		/* # of blocks in partition */
};
Void: use uint32_t [4].

daddr_t in these two is at least used. In the others it's not:

> - libtirpc
UPSTREAM GIT: daddr_t is defined [5], but I don't see it used anywhere. But caddr_t is used heavily. Unfortunately libtirpc upstream is not responding much, if anyone wants to remove daddr_t in upstream.
Void: enables it, but IMHO could be removed.

> - sleuthkit
UPSTREAM GIT: Again, defined [7], but not used.
Void: use uint32_t [8].


Kind regards,
Petr

[1] https://github.com/baruch/gpart/blob/master/src/gm_s86dl.h#L43
[2] https://github.com/void-linux/void-packages/blob/master/srcpkgs/gpart/patches/musl_loff_t.patch
[3] https://git.opensvc.com/gitweb.cgi?p=multipath-tools/.git;a=blob;f=kpartx/solaris.c;h=8c1a971be698675d3c99ef83a5958bc27099c73a;hb=HEAD
[4] https://github.com/void-linux/void-packages/blob/master/srcpkgs/kpartx/template#L20
[5] http://git.linux-nfs.org/?p=steved/libtirpc.git;a=blob;f=tirpc/rpc/types.h;h=f069efa77d1da598eba99d637107f971036ca535;hb=HEAD#l85
[6] https://github.com/void-linux/void-packages/blob/master/srcpkgs/libtirpc/patches/musl.patch
[7] https://github.com/sleuthkit/sleuthkit/blob/develop/tsk/base/tsk_os.h#L55
[8] https://github.com/void-linux/void-packages/blob/master/srcpkgs/sleuthkit/template
Rich Felker June 10, 2019, 8:30 p.m.
On Fri, Jun 07, 2019 at 11:25:45AM -0400, Rich Felker wrote:
> On Fri, Jun 07, 2019 at 02:52:07PM +0200, Leah Neukirchen wrote:
> > Rich Felker <dalias@libc.org> writes:
> > 
> > > On Fri, Jun 07, 2019 at 07:53:29AM +0200, Petr Vorel wrote:
> > >> Hi Rich,
> > >> 
> > >> ....
> > >> > > +++ b/include/sys/types.h
> > >> > > @@ -29,6 +29,7 @@ extern "C" {
> > >> > >  #define __NEED_clock_t
> > >> > >  #define __NEED_suseconds_t
> > >> > >  #define __NEED_blksize_t
> > >> > > +#define __NEED_daddr_t
> > >> ....
> > >> 
> > >> > daddr_t is not a standard type, so can't be exposed by default here
> > >> > (aside from the dubious "*_t is always reserved" rule), and it's only
> > >> So should it be wrapped by #if defined(_GNU_SOURCE) || defined(_BSD_SOURCE) ?
> > >> 
> > >> > proposed to be used in one header, so it doesn't belong in alltypes.h
> > >> > either.
> > >> Where should it be then? Shell I create bits/types.h for it?
> > >> The goal is to be loadable from <sys/types.h>
> > >
> > > Why? It's not a reasonable type for any application to use -- we've
> > > never gotten a report that something failed to build because of its
> > > absence, and even if we did, it would almost surely be a case of "fix
> > > the application".
> > 
> > FWIW, Void patches four packages that use daddr_t:
> > 
> > - gpart
> > - kpartx
> > - libtirpc
> > - sleuthkit
> > 
> > Additionally, five packages use caddr_t:
> > 
> > - ifenslave
> > - lwipv6
> > - macchanger
> > - sbcl
> > - virtuoso
> 
> Thanks. Can you tell if they have any legitimate motivation for doing
> so? What even is the conceptual definition of daddr_t? AIUI, caddr_t
> was historically something like void * or uintptr_t, a type for
> abstract addresses that might not actually point to objects, that
> mainly existed because void * didn't yet exist, but I have no idea
> what daddr_t is supposed to be.

OK, some Google-fu turns up that the historical meaning of daddr_t was
"disk address" (as opposed to caddr_t which AIUI means "core
address"). In that light, defining it would be completely wrong, since
the definition should match off_t (long long), not int/long, but
changing it to match would break any actual historical use of it...

This should probably be treated as a bug against the mtio interface
definition...

Rich