Paul Eggert wrote:
> 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
Hi Paul,
Thanks for the review.
> 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?
The ioctl is available/usable in 2.6.27 and newer that do not publish
this file. For example, it's in F13's (2.6.33's) /usr/include/linux/fiemap.h,
as well as the one in debian unstable's 2.6.32, but probably
not in much older kernels.
Hmm.. I see that it's available even in F11's 2.6.30.9-x
It would be better to include <linux/fiemap.h> when present,
else our copy of that header. Then, eventually, the else
clause will become unused. Note that even on newer kernels,
the linux/* headers are optional.
Eventually we'll have a hard requirement on kernel headers --
at least when building against a linux kernel.
>> 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.
Included in the patch below.
>> 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.
Good point. Thanks.
We can definitely avoid that allocation.
Do you feel like writing the patch?
I've just pushed this series to a branch, "fiemap-copy",
so others can follow along and contribute more easily.
>> 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);
I've done this in your name:
>From fffa2e8661a27978927fcc8afb6873631a753292 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 9 Jun 2010 08:15:07 +0200
Subject: [PATCH] copy.c: ensure proper alignment of fiemap buffer
* src/copy.c (fiemap_copy): Ensure that our fiemap buffer
is large enough and well-aligned.
Replace "0LL" with equivalent "0" as 3rd argument to lseek.
---
src/copy.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index f629771..27e083a 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -171,11 +171,12 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
char const *dst_name, bool *normal_copy_required)
{
bool last = false;
- char fiemap_buf[4096];
- struct fiemap *fiemap = (struct fiemap *) fiemap_buf;
+ union { struct fiemap f; char c[4096]; } fiemap_buf;
+ struct fiemap *fiemap = &fiemap_buf.f;
struct fiemap_extent *fm_ext = &fiemap->fm_extents[0];
- uint32_t count = ((sizeof fiemap_buf - sizeof (*fiemap))
- / sizeof (struct fiemap_extent));
+ enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_ext };
+ verify (count != 0);
+
off_t last_ext_logical = 0;
uint64_t last_ext_len = 0;
uint64_t last_read_size = 0;
@@ -185,7 +186,7 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
/* This is required at least to initialize fiemap->fm_start,
but also serves (in mid 2010) to appease valgrind, which
appears not to know the semantics of the FIEMAP ioctl. */
- memset (fiemap_buf, 0, sizeof fiemap_buf);
+ memset (&fiemap_buf, 0, sizeof fiemap_buf);
do
{
@@ -220,13 +221,13 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
off_t ext_logical = fm_ext[i].fe_logical;
uint64_t ext_len = fm_ext[i].fe_length;
- if (lseek (src_fd, ext_logical, SEEK_SET) < 0LL)
+ if (lseek (src_fd, ext_logical, SEEK_SET) < 0)
{
error (0, errno, _("cannot lseek %s"), quote (src_name));
return false;
}
- if (lseek (dest_fd, ext_logical, SEEK_SET) < 0LL)
+ if (lseek (dest_fd, ext_logical, SEEK_SET) < 0)
{
error (0, errno, _("cannot lseek %s"), quote (dst_name));
return false;
--
1.7.1.501.g23b46
Also, I've squashed this clean-up patch onto Jeff's original,
since ext_len is unsigned (of type uint64_t).
>From bad13e737c683757a2ed05404564d8863c5da30e Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyer...@redhat.com>
Date: Wed, 9 Jun 2010 08:24:39 +0200
Subject: [PATCH] remove 0 <
---
src/copy.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/copy.c b/src/copy.c
index 27e083a..f149be4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -240,7 +240,7 @@ fiemap_copy (int src_fd, int dest_fd, size_t buf_size,
last = true;
}
- while (0 < ext_len)
+ while (ext_len)
{
char buf[buf_size];
--
1.7.1.501.g23b46
