[2/2] define mt_fileno and mt_fileno struct mtget members as mt_blkno

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

Details

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

Commit Message

Petr Vorel June 7, 2019, 5:14 a.m.
following kernel definition, this fixes size on mips.

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 include/sys/mtio.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/sys/mtio.h b/include/sys/mtio.h
index f16a529b..4dd4c548 100644
--- a/include/sys/mtio.h
+++ b/include/sys/mtio.h
@@ -54,8 +54,8 @@  struct mtget {
 	long mt_dsreg;
 	long mt_gstat;
 	long mt_erreg;
-	int mt_fileno;
-	int mt_blkno;
+	daddr_t mt_fileno;
+	daddr_t mt_blkno;
 };
 
 #define MT_ISUNKNOWN		0x01

Comments

Rich Felker June 7, 2019, 5:31 a.m.
On Fri, Jun 07, 2019 at 07:14:44AM +0200, Petr Vorel wrote:
> following kernel definition, this fixes size on mips.
> 
> Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
> ---
>  include/sys/mtio.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sys/mtio.h b/include/sys/mtio.h
> index f16a529b..4dd4c548 100644
> --- a/include/sys/mtio.h
> +++ b/include/sys/mtio.h
> @@ -54,8 +54,8 @@ struct mtget {
>  	long mt_dsreg;
>  	long mt_gstat;
>  	long mt_erreg;
> -	int mt_fileno;
> -	int mt_blkno;
> +	daddr_t mt_fileno;
> +	daddr_t mt_blkno;
>  };
>  
>  #define MT_ISUNKNOWN		0x01
> -- 
> 2.21.0

Can you explain what problem this is supposed to fix? It definitely
needs further discussion to determine what the right way is, but
that's impossible without knowing the problem you're trying to solve.

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

...
> > +++ b/include/sys/mtio.h
> > @@ -54,8 +54,8 @@ struct mtget {
> >  	long mt_dsreg;
> >  	long mt_gstat;
> >  	long mt_erreg;
> > -	int mt_fileno;
> > -	int mt_blkno;
> > +	daddr_t mt_fileno;
> > +	daddr_t mt_blkno;
...

> Can you explain what problem this is supposed to fix? It definitely
> needs further discussion to determine what the right way is, but
> that's impossible without knowing the problem you're trying to solve.
Thanks for review. Not a problem for me actually. I've noticed, that glibc (and
thus uclibc-ng) and bionic follow kernel sources, which defines it long for mips
(and for sparc, but musl doesn't support it). Default is int [2]. Drop this
patch if this is not an issue.

> Rich

Kind regards,
Petr

[1] https://elixir.bootlin.com/linux/latest/source/arch/mips/include/uapi/asm/posix_types.h#L21
[2] https://elixir.bootlin.com/linux/latest/source/include/uapi/asm-generic/posix_types.h#L45
Rich Felker June 7, 2019, 5:50 a.m.
On Fri, Jun 07, 2019 at 07:42:15AM +0200, Petr Vorel wrote:
> Hi Rich,
> 
> ....
> > > +++ b/include/sys/mtio.h
> > > @@ -54,8 +54,8 @@ struct mtget {
> > >  	long mt_dsreg;
> > >  	long mt_gstat;
> > >  	long mt_erreg;
> > > -	int mt_fileno;
> > > -	int mt_blkno;
> > > +	daddr_t mt_fileno;
> > > +	daddr_t mt_blkno;
> ....
> 
> > Can you explain what problem this is supposed to fix? It definitely
> > needs further discussion to determine what the right way is, but
> > that's impossible without knowing the problem you're trying to solve.
> Thanks for review. Not a problem for me actually. I've noticed, that glibc (and
> thus uclibc-ng) and bionic follow kernel sources, which defines it long for mips
> (and for sparc, but musl doesn't support it). Default is int [2]. Drop this
> patch if this is not an issue.

Well it might be a problem on some archs. I think it's worth looking
into. We might need to add bits/mtio.h to define it appropriately for
the arch.

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

> > ....
> > > > +++ b/include/sys/mtio.h
> > > > @@ -54,8 +54,8 @@ struct mtget {
> > > >  	long mt_dsreg;
> > > >  	long mt_gstat;
> > > >  	long mt_erreg;
> > > > -	int mt_fileno;
> > > > -	int mt_blkno;
> > > > +	daddr_t mt_fileno;
> > > > +	daddr_t mt_blkno;
> > ....

> > > Can you explain what problem this is supposed to fix? It definitely
> > > needs further discussion to determine what the right way is, but
> > > that's impossible without knowing the problem you're trying to solve.
> > Thanks for review. Not a problem for me actually. I've noticed, that glibc (and
> > thus uclibc-ng) and bionic follow kernel sources, which defines it long for mips
> > (and for sparc, but musl doesn't support it). Default is int [2]. Drop this
> > patch if this is not an issue.

> Well it might be a problem on some archs. I think it's worth looking
> into. We might need to add bits/mtio.h to define it appropriately for
> the arch.
I'd be for bits/mtio.h as I suppose this problem still persists on mips.
But I'm not able to verify, just follow what it's defined in
kernel/glibc/uclibc/bionic.

My main concern is to have daddr_t defined in musl.

> Rich

Kind regards,
Petr
Rich Felker June 7, 2019, 6:01 a.m.
On Fri, Jun 07, 2019 at 07:58:29AM +0200, Petr Vorel wrote:
> Hi Rich,
> 
> > > ....
> > > > > +++ b/include/sys/mtio.h
> > > > > @@ -54,8 +54,8 @@ struct mtget {
> > > > >  	long mt_dsreg;
> > > > >  	long mt_gstat;
> > > > >  	long mt_erreg;
> > > > > -	int mt_fileno;
> > > > > -	int mt_blkno;
> > > > > +	daddr_t mt_fileno;
> > > > > +	daddr_t mt_blkno;
> > > ....
> 
> > > > Can you explain what problem this is supposed to fix? It definitely
> > > > needs further discussion to determine what the right way is, but
> > > > that's impossible without knowing the problem you're trying to solve.
> > > Thanks for review. Not a problem for me actually. I've noticed, that glibc (and
> > > thus uclibc-ng) and bionic follow kernel sources, which defines it long for mips
> > > (and for sparc, but musl doesn't support it). Default is int [2]. Drop this
> > > patch if this is not an issue.
> 
> > Well it might be a problem on some archs. I think it's worth looking
> > into. We might need to add bits/mtio.h to define it appropriately for
> > the arch.
> I'd be for bits/mtio.h as I suppose this problem still persists on mips.
> But I'm not able to verify, just follow what it's defined in
> kernel/glibc/uclibc/bionic.
> 
> My main concern is to have daddr_t defined in musl.

I don't follow why this is your goal/concern. It's not a standard
type, but some historical baggage from the 70s or 80s that some
implementations carried forward. It doesn't seem to meet any of musl's
criteria for inclusion.

If you think this is incorrect, please explain why you think it's
important to have it rather than just assuming it is.

Rich