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);
