GNU bug report logs - #6131
[PATCH]: fiemap support for efficient sparse file copy

Previous Next

Full log

Message #116 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> CS.UCLA.EDU>
To: Jim Meyering <jim <at> meyering.net>
Cc: Sunil Mushran <sunil.mushran <at> oracle.com>, bug-coreutils <at> gnu.org,
	Joel Becker <Joel.Becker <at> oracle.com>,
	"jeff.liu" <jeff.liu <at> oracle.com>, Tao Ma <tao.ma <at> oracle.com>,
	Chris Mason <chris.mason <at> oracle.com>
Subject: Re: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Tue, 08 Jun 2010 17:44:25 -0700

Jeff Liu and Jim Meyering wrote:

> diff --git a/src/fiemap.h b/src/fiemap.h
> new file mode 100644
> index 0000000..d33293b
> --- /dev/null
> +++ b/src/fiemap.h

Why is this file necessary?  fiemap.h is included only if it exists,
right?  Shouldn't we just use the kernel's fiemap.h rather than
copying it here and assuming kernel internals?

>          if (lseek (src_fd, ext_logical, SEEK_SET) < 0LL)

For this sort of thing, please just use "0" rather than "0LL".
"0" is easier to read and it has the same effect here.

>              char buf[buf_size];

This assumes C99, since buf_size is not known at compile time.
Also, isn't there a potential segmentation-violation problem
if buf_size is sufficiently large?

More generally, since the caller is already allocating a buffer of the
appropiate size, shouldn't we just reuse that buffer, rather than
allocating a new one?  That would avoid the problems of assuming
C99 and possible segmentation violations.


>   char fiemap_buf[4096];
>   struct fiemap *fiemap = (struct fiemap *) fiemap_buf;
>   struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
>   uint32_t count = ((sizeof fiemap_buf - sizeof (*fiemap))
>                     / sizeof (struct fiemap_extent));

This code isn't portable, since fiemap_buf is only char-aligned, and
struct fiemap may well require stricter alignment.  The code will work
on the x86 despite the alignment problem, but that's just a happy
coincidence.

A lesser point: the code assumes that 'struct fiemap' is sufficiently
small (considerably less than 4096 bytes in size); I expect that this
is universally true but we might as well check this assumption, since
it's easy to do so without any runtime overhead.

So I propose something like this instead:

   union { struct fiemap f; char c[4096]; } fiemap_buf;
   struct fiemap *fiemap = &fiemap_buf.f;
   struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
   enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_ext };
   verify (count != 0);

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.