[PATCH] xfsdump: don't assume getdents exists

[PATCH] xfsdump: don't assume getdents exists

[Kyle McMartin]

New Linux ports are using a standard syscall list that does not include
deprecated syscalls where 64-bit clean alternatives exist. As a result,
on arm64, __NR_getdents is undefined, resulting in xfsdump failing to
build.

To avoid that, in the case where __NR_getdents is unset in unistd.h,
avoid building the fallback path entirely, since
__ASSUME_GETDENTS64_SYSCALL will be true, the SYS_getdents64 case will
be the primary (and only) path used.

Signed-off-by: Kyle McMartin <kyle@redhat.com>

--- a/common/getdents.c
+++ b/common/getdents.c
@@ -70,6 +70,9 @@ extern int __have_no_getdents64;
 # ifndef SYS_getdents64
 #  define SYS_getdents64 __NR_getdents64
 # endif
+# ifndef __NR_getdents
+#  define __ONLY_GETDENTS64_SYSCALL     1
+# endif
 #endif
 
 
@@ -207,6 +210,13 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
 # endif
     }
 #endif
+/* Newer Linux ports are not adding deprecated syscalls, so to avoid compile
+ * failures since SYS_getdents will be undefined, we check for that and only
+ * build the fall-back case if SYS_getdents is defined. We know that
+ * __ASSUME_GETDENTS64_SYSCALL is set, since __NR_getdents64 must exist on
+ * those platforms.
+ */
+#ifndef __ONLY_GETDENTS64_SYSCALL
   {
     size_t red_nbytes;
     struct kernel_dirent *skdp, *kdp;
@@ -265,4 +275,5 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
     }
 
   return (char *) dp - buf;
+#endif
 }

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Dave Chinner]

On Wed, Nov 06, 2013 at 04:36:35PM -0500, Kyle McMartin wrote:
> New Linux ports are using a standard syscall list that does not include
> deprecated syscalls where 64-bit clean alternatives exist. As a result,
> on arm64, __NR_getdents is undefined, resulting in xfsdump failing to
> build.
> 
> To avoid that, in the case where __NR_getdents is unset in unistd.h,
> avoid building the fallback path entirely, since
> __ASSUME_GETDENTS64_SYSCALL will be true, the SYS_getdents64 case will
> be the primary (and only) path used.
> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>
> 
> --- a/common/getdents.c
> +++ b/common/getdents.c

Oh, how that code makes my eyes bleed. :/

> @@ -70,6 +70,9 @@ extern int __have_no_getdents64;
>  # ifndef SYS_getdents64
>  #  define SYS_getdents64 __NR_getdents64
>  # endif
> +# ifndef __NR_getdents
> +#  define __ONLY_GETDENTS64_SYSCALL     1
> +# endif
>  #endif
>  
>  
> @@ -207,6 +210,13 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
>  # endif
>      }
>  #endif
> +/* Newer Linux ports are not adding deprecated syscalls, so to avoid compile
> + * failures since SYS_getdents will be undefined, we check for that and only
> + * build the fall-back case if SYS_getdents is defined. We know that
> + * __ASSUME_GETDENTS64_SYSCALL is set, since __NR_getdents64 must exist on
> + * those platforms.
> + */
> +#ifndef __ONLY_GETDENTS64_SYSCALL
>    {
>      size_t red_nbytes;
>      struct kernel_dirent *skdp, *kdp;
> @@ -265,4 +275,5 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
>      }
>  
>    return (char *) dp - buf;
> +#endif
>  }

It's not pretty - but that code has already broken the ugly stick so
it isn't making the situation any worse.

Acked-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Kyle McMartin]

On Thu, Nov 07, 2013 at 09:48:44AM +1100, Dave Chinner wrote:
> > +++ b/common/getdents.c
> 
> Oh, how that code makes my eyes bleed. :/
> 

Me too :\

> > @@ -70,6 +70,9 @@ extern int __have_no_getdents64;
> >  # ifndef SYS_getdents64
> >  #  define SYS_getdents64 __NR_getdents64
> >  # endif
> > +# ifndef __NR_getdents
> > +#  define __ONLY_GETDENTS64_SYSCALL     1
> > +# endif
> >  #endif
> >  
> >  
> > @@ -207,6 +210,13 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
> >  # endif
> >      }
> >  #endif
> > +/* Newer Linux ports are not adding deprecated syscalls, so to avoid compile
> > + * failures since SYS_getdents will be undefined, we check for that and only
> > + * build the fall-back case if SYS_getdents is defined. We know that
> > + * __ASSUME_GETDENTS64_SYSCALL is set, since __NR_getdents64 must exist on
> > + * those platforms.
> > + */
> > +#ifndef __ONLY_GETDENTS64_SYSCALL
> >    {
> >      size_t red_nbytes;
> >      struct kernel_dirent *skdp, *kdp;
> > @@ -265,4 +275,5 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
> >      }
> >  
> >    return (char *) dp - buf;
> > +#endif
> >  }
> 
> It's not pretty - but that code has already broken the ugly stick so
> it isn't making the situation any worse.
> 
> Acked-by: Dave Chinner <dchinner@redhat.com>

I'll try to clean it up and send a patch for that, using getdents64
wherever possible, and handling the fallback at runtime instead of
making things all ifdeffy... Just wanted to get things building in the
mean time. (Oh man the coding style there is janky...)

--Kyle

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Dave Chinner]

On Wed, Nov 06, 2013 at 05:56:20PM -0500, Kyle McMartin wrote:
> On Thu, Nov 07, 2013 at 09:48:44AM +1100, Dave Chinner wrote:
> > > +++ b/common/getdents.c
> > 
> > Oh, how that code makes my eyes bleed. :/
> > 
> 
> Me too :\
> 
> > > @@ -70,6 +70,9 @@ extern int __have_no_getdents64;
> > >  # ifndef SYS_getdents64
> > >  #  define SYS_getdents64 __NR_getdents64
> > >  # endif
> > > +# ifndef __NR_getdents
> > > +#  define __ONLY_GETDENTS64_SYSCALL     1
> > > +# endif
> > >  #endif
> > >  
> > >  
> > > @@ -207,6 +210,13 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
> > >  # endif
> > >      }
> > >  #endif
> > > +/* Newer Linux ports are not adding deprecated syscalls, so to avoid compile
> > > + * failures since SYS_getdents will be undefined, we check for that and only
> > > + * build the fall-back case if SYS_getdents is defined. We know that
> > > + * __ASSUME_GETDENTS64_SYSCALL is set, since __NR_getdents64 must exist on
> > > + * those platforms.
> > > + */
> > > +#ifndef __ONLY_GETDENTS64_SYSCALL
> > >    {
> > >      size_t red_nbytes;
> > >      struct kernel_dirent *skdp, *kdp;
> > > @@ -265,4 +275,5 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
> > >      }
> > >  
> > >    return (char *) dp - buf;
> > > +#endif
> > >  }
> > 
> > It's not pretty - but that code has already broken the ugly stick so
> > it isn't making the situation any worse.
> > 
> > Acked-by: Dave Chinner <dchinner@redhat.com>
> 
> I'll try to clean it up and send a patch for that, using getdents64
> wherever possible, and handling the fallback at runtime instead of
> making things all ifdeffy... Just wanted to get things building in the
> mean time. (Oh man the coding style there is janky...)

I'd factor and reformat the code using the normal kernel style. :)

And to avoid ifdef hackery and runtime fallbacks, I'd add an
autoconf macro to determine which getdents call is supported, and
then only compile in the relevant function and not have to care
about runtime fallbacks.

But that may end up more complex and more work than you had in mind,
so feel free to ignore my suggestions ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Kyle McMartin]

On Thu, Nov 07, 2013 at 10:17:13AM +1100, Dave Chinner wrote:
> > I'll try to clean it up and send a patch for that, using getdents64
> > wherever possible, and handling the fallback at runtime instead of
> > making things all ifdeffy... Just wanted to get things building in the
> > mean time. (Oh man the coding style there is janky...)
> 
> I'd factor and reformat the code using the normal kernel style. :)
> 
> And to avoid ifdef hackery and runtime fallbacks, I'd add an
> autoconf macro to determine which getdents call is supported, and
> then only compile in the relevant function and not have to care
> about runtime fallbacks.
> 
> But that may end up more complex and more work than you had in mind,
> so feel free to ignore my suggestions ;)
> 

Good thinking... I'll poke at it.

Thanks Dave!

Kyle

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Christoph Hellwig]

On Wed, Nov 06, 2013 at 04:36:35PM -0500, Kyle McMartin wrote:
> New Linux ports are using a standard syscall list that does not include
> deprecated syscalls where 64-bit clean alternatives exist. As a result,
> on arm64, __NR_getdents is undefined, resulting in xfsdump failing to
> build.
> 
> To avoid that, in the case where __NR_getdents is unset in unistd.h,
> avoid building the fallback path entirely, since
> __ASSUME_GETDENTS64_SYSCALL will be true, the SYS_getdents64 case will
> be the primary (and only) path used.

Seems like we could drop this mess of a file entirely and just rely
on getdents64 from glibc.  The raison d'etre of that file appears to
support 64 bit getdents before glibc really supported it, and I don't
think we'll have to support such old configurations anymore.  I'll give
it a quick spin and see how that works out.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Christoph Hellwig]

On Wed, Nov 06, 2013 at 10:58:35PM -0800, Christoph Hellwig wrote:
> On Wed, Nov 06, 2013 at 04:36:35PM -0500, Kyle McMartin wrote:
> > New Linux ports are using a standard syscall list that does not include
> > deprecated syscalls where 64-bit clean alternatives exist. As a result,
> > on arm64, __NR_getdents is undefined, resulting in xfsdump failing to
> > build.
> > 
> > To avoid that, in the case where __NR_getdents is unset in unistd.h,
> > avoid building the fallback path entirely, since
> > __ASSUME_GETDENTS64_SYSCALL will be true, the SYS_getdents64 case will
> > be the primary (and only) path used.
> 
> Seems like we could drop this mess of a file entirely and just rely
> on getdents64 from glibc.  The raison d'etre of that file appears to
> support 64 bit getdents before glibc really supported it, and I don't
> think we'll have to support such old configurations anymore.  I'll give
> it a quick spin and see how that works out.

Turns out there is no glibc wrapper for getdents, so unless we want
to use the buffered readdir code we'll have to stick to some variant of
this.  But I think we can just kill the fallback and use getdents64
unconditionally.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Eric Sandeen]

On 11/6/13, 3:36 PM, Kyle McMartin wrote:
> New Linux ports are using a standard syscall list that does not include
> deprecated syscalls where 64-bit clean alternatives exist. As a result,
> on arm64, __NR_getdents is undefined, resulting in xfsdump failing to
> build.
> 
> To avoid that, in the case where __NR_getdents is unset in unistd.h,
> avoid building the fallback path entirely, since
> __ASSUME_GETDENTS64_SYSCALL will be true, the SYS_getdents64 case will
> be the primary (and only) path used.
> 
> Signed-off-by: Kyle McMartin <kyle@redhat.com>

Hey Kyle - were you going to do a V2 of this one?

Just checking, thanks - 
-Eric

> --- a/common/getdents.c
> +++ b/common/getdents.c
> @@ -70,6 +70,9 @@ extern int __have_no_getdents64;
>  # ifndef SYS_getdents64
>  #  define SYS_getdents64 __NR_getdents64
>  # endif
> +# ifndef __NR_getdents
> +#  define __ONLY_GETDENTS64_SYSCALL     1
> +# endif
>  #endif
>  
>  
> @@ -207,6 +210,13 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
>  # endif
>      }
>  #endif
> +/* Newer Linux ports are not adding deprecated syscalls, so to avoid compile
> + * failures since SYS_getdents will be undefined, we check for that and only
> + * build the fall-back case if SYS_getdents is defined. We know that
> + * __ASSUME_GETDENTS64_SYSCALL is set, since __NR_getdents64 must exist on
> + * those platforms.
> + */
> +#ifndef __ONLY_GETDENTS64_SYSCALL
>    {
>      size_t red_nbytes;
>      struct kernel_dirent *skdp, *kdp;
> @@ -265,4 +275,5 @@ getdents_wrap (int fd, char *buf, size_t nbytes)
>      }
>  
>    return (char *) dp - buf;
> +#endif
>  }
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Kyle McMartin]

On Tue, Dec 10, 2013 at 08:46:08AM -0600, Eric Sandeen wrote:
> On 11/6/13, 3:36 PM, Kyle McMartin wrote:
> > New Linux ports are using a standard syscall list that does not include
> > deprecated syscalls where 64-bit clean alternatives exist. As a result,
> > on arm64, __NR_getdents is undefined, resulting in xfsdump failing to
> > build.
> > 
> > To avoid that, in the case where __NR_getdents is unset in unistd.h,
> > avoid building the fallback path entirely, since
> > __ASSUME_GETDENTS64_SYSCALL will be true, the SYS_getdents64 case will
> > be the primary (and only) path used.
> > 
> > Signed-off-by: Kyle McMartin <kyle@redhat.com>
> 
> Hey Kyle - were you going to do a V2 of this one?
> 
> Just checking, thanks - 
> -Eric
> 

Yeah, I can do that today. Thanks Eric.

--Kyle

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Kyle McMartin]

On Tue, Dec 10, 2013 at 08:46:08AM -0600, Eric Sandeen wrote:
> Hey Kyle - were you going to do a V2 of this one?
> 
> Just checking, thanks - 

i've got a pair of somewhat more substantial patches to fix the ifdef
hell in this file, clean things up so it works on both getdents64 only
and not, and then rips out the pre-2.4.1 getdents code.

https://github.com/jkkm/xfsdump/commit/e156bcf06effbf5f2e26c53efb0ce028c4281f9e
https://github.com/jkkm/xfsdump/commit/fff56b471220a8fa305b9158c4f8f0c7344c8fe6

the latter looks quite a bit better... care to take a quick look and if
they're decent, i'll submit them both.

--kyle

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] xfsdump: don't assume getdents exists

[Dave Chinner]

On Sat, Mar 22, 2014 at 01:12:05AM -0400, Kyle McMartin wrote:
> On Tue, Dec 10, 2013 at 08:46:08AM -0600, Eric Sandeen wrote:
> > Hey Kyle - were you going to do a V2 of this one?
> > 
> > Just checking, thanks - 
> 
> i've got a pair of somewhat more substantial patches to fix the ifdef
> hell in this file, clean things up so it works on both getdents64 only
> and not, and then rips out the pre-2.4.1 getdents code.
> 
> https://github.com/jkkm/xfsdump/commit/e156bcf06effbf5f2e26c53efb0ce028c4281f9e
> https://github.com/jkkm/xfsdump/commit/fff56b471220a8fa305b9158c4f8f0c7344c8fe6
> 
> the latter looks quite a bit better... care to take a quick look and if
> they're decent, i'll submit them both.

Please just post the patches. People will comment on them on them
once you post them to the list...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs