GNU bug report logs -
#9077
coreutils-8.12: fiemap.h uses "struct fiemap_extent fm_extents[0];"
Previous Next
To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 9077 in the body.
You can then email your comments to 9077 AT debbugs.gnu.org in the normal way.
Toggle the display of automated, internal messages from the tracker.
Report forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9077; Package
coreutils.
(Thu, 14 Jul 2011 07:05:02 GMT)
Full text and
rfc822 format available.
Acknowledgement sent
to
"Joachim Schmitz" <jojo <at> schmitz-digital.de>:
New bug report received and forwarded. Copy sent to
bug-coreutils <at> gnu.org.
(Thu, 14 Jul 2011 07:05:02 GMT)
Full text and
rfc822 format available.
Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
[Message part 1 (text/plain, inline)]
Hi folks
fiemap.h uses
struct fiemap_extent fm_extents[0];
My compiler (HP NonStop) disallows that. The following would be allowed
though:
struct fiemap_extent fm_extents[];
Bye, Jojo
[Message part 2 (text/html, inline)]
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9077; Package
coreutils.
(Thu, 14 Jul 2011 11:48:02 GMT)
Full text and
rfc822 format available.
Message #8 received at 9077 <at> debbugs.gnu.org (full text, mbox):
On 14/07/11 08:03, Joachim Schmitz wrote:
> Hi folks
>
> fiemap.h uses
>
> struct fiemap_extent fm_extents[0];
>
> My compiler (HP NonStop) disallows that. The following would be allowed
> though:
>
> struct fiemap_extent fm_extents[];
The non standard "zero length array" syntax was copied from
the linux kernel headers. Changing this to a C99 flexible array
as you suggest, should be fine according to the constraints listed here:
http://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
It results in identical object files with GCC here.
I'll apply the above in your name.
cheers,
Pádraig.
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9077; Package
coreutils.
(Thu, 14 Jul 2011 22:51:02 GMT)
Full text and
rfc822 format available.
Message #11 received at 9077 <at> debbugs.gnu.org (full text, mbox):
On 07/14/11 04:45, Pádraig Brady wrote:
> The non standard "zero length array" syntax was copied from
> the linux kernel headers. Changing this to a C99 flexible array
> as you suggest, should be fine according to the constraints listed here:
Won't this break non-GCC compilers that don't support
C99 flexible arrays?
Instead, how about this patch? If I understand things correctly,
it should fix the problem on NonStop, and should also work with
pre-C99 non-GCC compilers.
diff --git a/src/fiemap.h b/src/fiemap.h
index 1938c74..e7243b4 100644
--- a/src/fiemap.h
+++ b/src/fiemap.h
@@ -51,8 +51,9 @@ struct fiemap
uint32_t fm_reserved;
- /* Array of mapped extents(out). */
- struct fiemap_extent fm_extents[0];
+ /* Array of mapped extents(out).
+ The actual size is given by fm_extent_count. */
+ struct fiemap_extent fm_extents[1];
};
/* The maximum offset can be mapped for a file. */
Information forwarded
to
owner <at> debbugs.gnu.org, bug-coreutils <at> gnu.org:
bug#9077; Package
coreutils.
(Fri, 15 Jul 2011 01:14:01 GMT)
Full text and
rfc822 format available.
Message #14 received at 9077 <at> debbugs.gnu.org (full text, mbox):
On 14/07/11 23:50, Paul Eggert wrote:
> On 07/14/11 04:45, Pádraig Brady wrote:
>> The non standard "zero length array" syntax was copied from
>> the linux kernel headers. Changing this to a C99 flexible array
>> as you suggest, should be fine according to the constraints listed here:
>
> Won't this break non-GCC compilers that don't support
> C99 flexible arrays?
Yes. But we require C99, and it would be
interesting to see what features we could rely
on going forward.
I had checked with solaris compilers which support
this since 2004 at least.
Though saying that, the single element hack
isn't too bad and might allow some other platform to build
without issue.
> Instead, how about this patch? If I understand things correctly,
> it should fix the problem on NonStop, and should also work with
> pre-C99 non-GCC compilers.
feel free to push that
cheers,
Pádraig.
Reply sent
to
Paul Eggert <eggert <at> cs.ucla.edu>:
You have taken responsibility.
(Fri, 15 Jul 2011 05:09:02 GMT)
Full text and
rfc822 format available.
Notification sent
to
"Joachim Schmitz" <jojo <at> schmitz-digital.de>:
bug acknowledged by developer.
(Fri, 15 Jul 2011 05:09:02 GMT)
Full text and
rfc822 format available.
Message #19 received at 9077-done <at> debbugs.gnu.org (full text, mbox):
On 07/14/11 18:11, Pádraig Brady wrote:
> feel free to push that
OK, thanks, done.
Message #20 received at 9077-done <at> debbugs.gnu.org (full text, mbox):
On 15/07/11 06:08, Paul Eggert wrote:
> On 07/14/11 18:11, Pádraig Brady wrote:
>> feel free to push that
>
> OK, thanks, done.
And now that I look at the code a little longer,
I think that the sizeof calculations need to be
adjusted in extent_scan_read() too.
But that's a step too far I think.
So I'm going to apply this.
cheers,
Pádraig.
commit 1ab94c86ca409c2800ce39e2934538c1eed2af10
Author: Pádraig Brady <P <at> draigBrady.com>
Date: Fri Jul 15 09:51:35 2011 +0100
build: avoid a fiemap compile failure on some systems
* src/fiemap.h (struct fiemap): Adjust the previous change
to the fiemap_extents array, which would also require changes
to the sizeof calculations in extent_scan_read().
Instead, only declare the fiemap_extents zero length array
on linux, which is the only platform that references this member.
This avoids a compilation failure on systems that don't support
this non standard construct. We don't use the equivalent C99
flexible array construct so as to have maximum portability.
* src/extent-scan.c: Cleanup. Remove a redundant #ifndef.
diff --git a/src/extent-scan.c b/src/extent-scan.c
index 48dd564..37445b8 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -24,11 +24,9 @@
#include "system.h"
#include "extent-scan.h"
+#include "fiemap.h"
#include "xstrtol.h"
-#ifndef HAVE_FIEMAP
-# include "fiemap.h"
-#endif
/* Work around Linux kernel issues on BTRFS and EXT4 before 2.6.39.
FIXME: remove in 2013, or whenever we're pretty confident
diff --git a/src/fiemap.h b/src/fiemap.h
index e7243b4..3b35297 100644
--- a/src/fiemap.h
+++ b/src/fiemap.h
@@ -52,8 +52,12 @@ struct fiemap
uint32_t fm_reserved;
/* Array of mapped extents(out).
- The actual size is given by fm_extent_count. */
- struct fiemap_extent fm_extents[1];
+ This is protected by the ifdef because it uses non standard
+ zero length arrays. Note C99 has the equivalent flexible arrays,
+ but we don't use those for maximum portability to older systems. */
+#ifdef __linux__
+ struct fiemap_extent fm_extents[0];
+#endif
};
/* The maximum offset can be mapped for a file. */
Message #21 received at 9077-done <at> debbugs.gnu.org (full text, mbox):
On 07/15/11 02:49, Pádraig Brady wrote:
> +#ifdef __linux__
> + struct fiemap_extent fm_extents[0];
> +#endif
This assumes that all GNU/Linux compiles will
use a compiler that allows size-zero arrays;
also, it assumes that sizeof (struct fiemap)
== offsetof (struct fiemap, fm_extents).
Although both assumptions are probably
portable in practice in this particular case,
it's better to avoid them, if avoiding them is easy.
This will help future-proof the code if we alter the
data structures, as altering them might cause problems
such as what I recently encountered in GNU Emacs's C code.
How about the following further patch? It should do the trick.
build: more-portable fix for fiemap compile failure
* src/extent-scan.c: Include <stddef.h>, for offsetof.
(extent_scan_read): Don't assume that sizeof (struct fiemap) is
equal to offsetof (struct fiemap, fm_extents). Although this
should be a portable assumption in practice for this particular
case, the C standard doesn't guarantee it, and we might as well do
it the right way. As it happens I recently got burned by this
exact issue in the source code for GNU Emacs; see
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=8884>.
* src/fiemap.h (struct fiemap.fm_extents): Change size back to 1.
diff --git a/src/extent-scan.c b/src/extent-scan.c
index 37445b8..434258f 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -17,6 +17,7 @@
Written by Jie Liu (jeff.liu <at> oracle.com). */
#include <config.h>
+#include <stddef.h>
#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/utsname.h>
@@ -96,7 +97,10 @@ extent_scan_read (struct extent_scan *scan)
union { struct fiemap f; char c[4096]; } fiemap_buf;
struct fiemap *fiemap = &fiemap_buf.f;
struct fiemap_extent *fm_extents = &fiemap->fm_extents[0];
- enum { count = (sizeof fiemap_buf - sizeof *fiemap)/sizeof *fm_extents };
+ enum {
+ size = sizeof fiemap_buf - offsetof (struct fiemap, fm_extents),
+ count = size / sizeof *fm_extents
+ };
verify (count > 1);
/* This is required at least to initialize fiemap->fm_start,
diff --git a/src/fiemap.h b/src/fiemap.h
index 15ddff9..60561d3 100644
--- a/src/fiemap.h
+++ b/src/fiemap.h
@@ -52,12 +52,9 @@ struct fiemap
uint32_t fm_reserved;
/* Array of mapped extents(out).
- This is protected by the ifdef because it uses non standard
- zero length arrays. Note C99 has the equivalent flexible arrays,
- but we don't use those for maximum portability to older systems. */
-# ifdef __linux__
- struct fiemap_extent fm_extents[0];
-# endif
+ This uses a size 1 array to be compatible with compilers that
+ don't support C99's flexible arrays or GCC's size-zero arrays. */
+ struct fiemap_extent fm_extents[1];
};
/* The maximum offset can be mapped for a file. */
Message #22 received at 9077-done <at> debbugs.gnu.org (full text, mbox):
On 15/07/11 23:45, Paul Eggert wrote:
> On 07/15/11 02:49, Pádraig Brady wrote:
>> +#ifdef __linux__
>> + struct fiemap_extent fm_extents[0];
>> +#endif
>
> This assumes that all GNU/Linux compiles will
> use a compiler that allows size-zero arrays;
> also, it assumes that sizeof (struct fiemap)
> == offsetof (struct fiemap, fm_extents).
> Although both assumptions are probably
> portable in practice in this particular case,
> it's better to avoid them, if avoiding them is easy.
> This will help future-proof the code if we alter the
> data structures, as altering them might cause problems
> such as what I recently encountered in GNU Emacs's C code.
>
> How about the following further patch? It should do the trick.
I tested that and it didn't work.
Even though the *fiemap structs are the same in mem
with and without the change, the ioctl() is returning ENOTTY
when the array is of size 1. I guess this is because
the array element is unpacked for the ioctl which
is then failing kernel checks.
cheers,
Pádraig.
Message #23 received at 9077-done <at> debbugs.gnu.org (full text, mbox):
On 07/16/11 01:37, Pádraig Brady wrote:
> I guess this is because
> the array element is unpacked for the ioctl which
> is then failing kernel checks.
That's odd, since as far as I can see, ioctl should see exactly
the same bytes either way; the kernel doesn't know about the app's
array elements, and all it should care about are the bytes.
However, I've run out of ideas for debugging remotely. Perhaps this
will have to wait until someone runs into the portability problem
that I am merely hypothesizing. Thanks for looking into it, anyway.
bug archived.
Request was from
Debbugs Internal Request <help-debbugs <at> gnu.org>
to
internal_control <at> debbugs.gnu.org.
(Sat, 13 Aug 2011 11:24:05 GMT)
Full text and
rfc822 format available.
This bug report was last modified 14 years and 7 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.