FYI: questionable xfsdump code

FYI: questionable xfsdump code

[Marc Lehmann]

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.

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

Re: FYI: questionable xfsdump code

[Brian Foster]

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