Jim Meyering wrote:
> 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?
Thanks for pointing this out!
Hi Paul,
I am appreciated if you have time for writing the patch. Or else, I will
follow up sometime in the
next few days since I have an urgent task need to handle over at present.
Regards,
-Jeff
>
> 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
>
>
>
--
With Windows 7, Microsoft is asserting legal control over your computer and is
using this power to
abuse computer users.
