| Message ID | 20190708213551.26349-1-petr.vorel@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | [1/1] kpartx: Use __kernel_daddr_t for solaris_x86_slice.s_start | expand |
> -//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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 */ }; -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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/ -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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 */ };
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(-)