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@xxxxxxxxxx
> -=====/_/_//_/\_,_/ /_/\_\
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
|