[PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Petr Vorel]

It was meant to be used daddr_t (which is mostly int, only sparc and
mips have it defined as int), but instead used long.
But musl libc does not define daddr_t as it's deprecated, therefore
use __kernel_daddr_t from <linux/types.h>.

Signed-off-by: Petr Vorel <petr.vorel@gmail.com>
---
 kpartx/solaris.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kpartx/solaris.c b/kpartx/solaris.c
index 8c1a971b..e7826c62 100644
--- a/kpartx/solaris.c
+++ b/kpartx/solaris.c
@@ -1,17 +1,15 @@
 #include "kpartx.h"
 #include <stdio.h>
-#include <sys/types.h>
+#include <linux/types.h>
 #include <time.h>		/* time_t */
 
 #define SOLARIS_X86_NUMSLICE	8
 #define SOLARIS_X86_VTOC_SANE	(0x600DDEEEUL)
 
-//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 */
+	__kernel_daddr_t s_start;	/* start sector no of partition */
 	long		s_size;		/* # of blocks in partition */
 };
 
-- 
2.20.1

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Christoph Hellwig]

> -//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 */
> +	__kernel_daddr_t s_start;	/* start sector no of partition */
>  	long		s_size;		/* # of blocks in partition */
>  };

What this really should use is fixed size types.

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Petr Vorel]

Hi Christoph,

> > -//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 */
> > +	__kernel_daddr_t s_start;	/* start sector no of partition */
> >  	long		s_size;		/* # of blocks in partition */
> >  };

> What this really should use is fixed size types.
If it's not specific to __kernel_daddr_t nor daddr_t ("The type of a disk
address") and long is sufficient for all platforms, that's even better.

I'd be just for removing typedef int daddr_t comment.

BTW gpart also uses struct solaris_x86_slice, with daddr_t [1].
I've filed a PR [2], but I guess I'll change it to long.

Kind regards,
Petr

[1] https://github.com/baruch/gpart/blob/master/src/gm_s86dl.h#L43
[2] https://github.com/baruch/gpart/pull/15

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Christoph Hellwig]

On Tue, Jul 09, 2019 at 10:02:05AM +0200, Petr Vorel wrote:
> 
> > What this really should use is fixed size types.
> If it's not specific to __kernel_daddr_t nor daddr_t ("The type of a disk
> address") and long is sufficient for all platforms, that's even better.
> 
> I'd be just for removing typedef int daddr_t comment.
> 
> BTW gpart also uses struct solaris_x86_slice, with daddr_t [1].
> I've filed a PR [2], but I guess I'll change it to long.

solaris_x86_slice is an on-disk format, defined for good old 32-bit
x86 Solaris.  So the question is not if it is enough, but if it matches
what Solaris does.  I don't have the Solaris source at the moment,
but here is what the Linux kernel uses:

struct solaris_x86_slice {
        __le16 s_tag;           /* ID tag of partition */
        __le16 s_flag;          /* permission flags */
        __le32 s_start;         /* start sector no of partition */
        __le32 s_size;          /* # of blocks in partition */
};

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Petr Vorel]

Hi Christoph,

> On Tue, Jul 09, 2019 at 10:02:05AM +0200, Petr Vorel wrote:

> > > What this really should use is fixed size types.
> > If it's not specific to __kernel_daddr_t nor daddr_t ("The type of a disk
> > address") and long is sufficient for all platforms, that's even better.

> > I'd be just for removing typedef int daddr_t comment.

> > BTW gpart also uses struct solaris_x86_slice, with daddr_t [1].
> > I've filed a PR [2], but I guess I'll change it to long.

> solaris_x86_slice is an on-disk format, defined for good old 32-bit
> x86 Solaris.  So the question is not if it is enough, but if it matches
> what Solaris does.  I don't have the Solaris source at the moment,
> but here is what the Linux kernel uses:

> struct solaris_x86_slice {
>         __le16 s_tag;           /* ID tag of partition */
>         __le16 s_flag;          /* permission flags */
>         __le32 s_start;         /* start sector no of partition */
>         __le32 s_size;          /* # of blocks in partition */
> };

I tried to search in [1], with not much success, I don't know the original name
of the struct and struct members are quite similar. Do you have a tip, where it
could be or would you dare to search?

Christophe already merged my patch as
129e6fe6 ("kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start")

But, according to your comments it looks to me better to use the exact structure
kernel uses. So, if we don't find anything, I'd be for using kernel struct.

Kind regards,
Petr

[1] https://grok.elemental.org/source/

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Christoph Hellwig]

On Wed, Oct 02, 2019 at 08:05:09AM +0200, Petr Vorel wrote:
> I tried to search in [1], with not much success, I don't know the original name
> of the struct and struct members are quite similar. Do you have a tip, where it
> could be or would you dare to search?

No, I don't know Solaris very well.

> 
> Christophe already merged my patch as
> 129e6fe6 ("kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start")
> 
> But, according to your comments it looks to me better to use the exact structure
> kernel uses. So, if we don't find anything, I'd be for using kernel struct.

Thanks, that would be great.

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Petr Vorel]

Hi Christoph,

> On Wed, Oct 02, 2019 at 08:05:09AM +0200, Petr Vorel wrote:
> > I tried to search in [1], with not much success, I don't know the original name
> > of the struct and struct members are quite similar. Do you have a tip, where it
> > could be or would you dare to search?

> No, I don't know Solaris very well.

> > Christophe already merged my patch as
> > 129e6fe6 ("kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start")

> > But, according to your comments it looks to me better to use the exact structure
> > kernel uses. So, if we don't find anything, I'd be for using kernel struct.

> Thanks, that would be great.
I've sent a patch, as RFC, Cc also Baruch Even, the gpart maintainer.
I wonder, if there is anybody actually using this code nowadays.

Kind regards,
Petr

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Baruch Even]

[-- Attachment #1.1: Type: text/plain, Size: 1535 bytes --]

Hi,

The patch looks good to me, as for gpart, I needed it so I've taken the
bits and pieces that floated around and merged them all into one place from
all distributions that I could find.   As for users, in Debian the number
of users is around 250 so it's a very low usage program but I guess that
would always be unless very many people lost their partitions every day :-)

https://qa.debian.org/popcon-graph.php?packages=gpart

Cheers,
Baruch

P.S. While I'm sort of maintaining the program, it's only maintenance, no
active development is going on with it and very rarely do I get a patch or
an issue reported.

On Tue, Oct 8, 2019 at 11:18 AM Petr Vorel <petr.vorel@gmail.com> wrote:

> Hi Christoph,
>
> > On Wed, Oct 02, 2019 at 08:05:09AM +0200, Petr Vorel wrote:
> > > I tried to search in [1], with not much success, I don't know the
> original name
> > > of the struct and struct members are quite similar. Do you have a tip,
> where it
> > > could be or would you dare to search?
>
> > No, I don't know Solaris very well.
>
> > > Christophe already merged my patch as
> > > 129e6fe6 ("kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start")
>
> > > But, according to your comments it looks to me better to use the exact
> structure
> > > kernel uses. So, if we don't find anything, I'd be for using kernel
> struct.
>
> > Thanks, that would be great.
> I've sent a patch, as RFC, Cc also Baruch Even, the gpart maintainer.
> I wonder, if there is anybody actually using this code nowadays.
>
> Kind regards,
> Petr
>

[-- Attachment #1.2: Type: text/html, Size: 2166 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start

[Petr Vorel]

Hi Baruch,

> Hi,

> The patch looks good to me, as for gpart, I needed it so I've taken the
> bits and pieces that floated around and merged them all into one place from
> all distributions that I could find.   As for users, in Debian the number
> of users is around 250 so it's a very low usage program but I guess that
> would always be unless very many people lost their partitions every day :-)

> https://qa.debian.org/popcon-graph.php?packages=gpart

> Cheers,
> Baruch

> P.S. While I'm sort of maintaining the program, it's only maintenance, no
> active development is going on with it and very rarely do I get a patch or
> an issue reported.

Thanks for info. FYI (even you don't do active development), although this patch
was merged, it's IMHO wrong approach.
I've sent better approach [1] and I'm going to send v2, which uses uint{16,32}_t
instead of uint{16,32}_t (fixed size types are good enough).
I'll Cc you in new patch, but feel free to ignore it.

Kind regards,
Petr

[1] https://www.redhat.com/archives/dm-devel/2019-October/msg00058.html