Re: FYI: questionable xfsdump code

[Brian Foster <bfoster@redhat.com>]

On Tue, Jul 28, 2015 at 08:33:33AM +0200, Marc Lehmann wrote:
> Hi!
> 
> While causally browsing xfsdump code,I found this, in
> common/getdents.c:getdents_wrap (in xfsdump
> 
>         off64_t last_offset = -1;
> 
>         ...
> 
>         while ((char *)kdp < kbuf + retval) {
>                 ...
> 
>                 if ((sizeof(dp->d_ino) != sizeof(kdp->d_ino))
>                     || (sizeof(dp->d_off) != sizeof(kdp->d_off))) {
>                         /* Overflow.  If there was at least one entry
>                            before this one, return them without error,
>                            otherwise signal overflow.  */
>                         if (last_offset != -1) {
>                                 lseek64(fd, last_offset, SEEK_SET);
>                                 return (char *)dp - buf;
>                         }
>                         errno = EOVERFLOW;
>                         return -1;
>                 }
> 
>                 last_offset = d_off;
> 
>                 ...
>         }
> 
> While not necessarily a bug, this comment is very confused - there is no
> way to reach the code inside the if with last_offset != -1, as the if
> condition is a compiletime constant.
> 

It looks like this changed in:

	b1d6979f remove ancient sys_getdents code paths

It used to look like this:

		if ((sizeof (dp->d_ino) != sizeof (kdp->d_ino)
		     && dp->d_ino != d_ino)
		    || (sizeof (dp->d_off) != sizeof (kdp->d_off)
			&& dp->d_off != d_off))
		{
			...
		}

... which probably made more sense.

Brian


> This might be harmless dead code from some refactorisation gone wrong,
> or indicative of some bug due to some logic error. In any case, I just
> wanted to bring it to your attention.
> 
> And as a side note, memcpy would be more efficient here, especially as it
> is called very often, (and especially so on irix :-):
> 
>                 memmove(dp->d_name, kdp->d_name,
>                         old_reclen - offsetof(struct kernel_dirent64, d_name));
> 
> -- 
>                 The choice of a       Deliantra, the free code+content MORPG
>       -----==-     _GNU_              http://www.deliantra.net
>       ----==-- _       generation
>       ---==---(_)__  __ ____  __      Marc Lehmann
>       --==---/ / _ \/ // /\ \/ /      schmorp@schmorp.de
>       -=====/_/_//_/\_,_/ /_/\_\
> 
> _______________________________________________
> 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