About errors in XFS header files and xfsprogs

About errors in XFS header files and xfsprogs

[Bogdan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

 Hello.

 The XFS library header files contained in xfsprogs (2.9.3-1) contain
many errors. Run the following command on a Linux system to verify:

gcc -Wall -Wextra -Wfloat-equal -Wbad-function-cast -Wsign-compare
- -Wunreachable-code -Wpointer-arith -Wcast-qual -Wcast-align
- -Wstrict-prototypes -Wformat-security -Wformat-nonliteral
- -Wnested-externs -Wshadow -Wconversion -Wdeclaration-after-statement
- -Wundef -Wpadded -Wredundant-decls -pedantic -c -o xfs_repair.o
xfs_repair.c

(you can use any file which #includes the libxfs.h header). The use of
'long long' is less important. The header files should not only be
ISO-C-compatible, but even ANSI-C-compatible. This will allow anyone
to use them.

 Please, make useful public functions available in header files.
Especially platform_check_ismounted() and platform_check_iswritable()
and other "platform" functions.

 Also, anything that is a part of the public interface, should be in
the "xfs namespace", i.e. start with "xfs_". At least one issue of
this type is known: the list_head structure, declared in other files
from other libraries. Things like these are very important to fix,
because they are making other header files useless.

 You shouldn't handle gettext and internationalization in header
files. Let users do it by themselves. Otherwise, a double-defined
errors occur for the "_" macro.

 Also, a following error occurs in the library:

 gcc -g -O2 -DDEBUG -DVERSION=\"2.9.3\"
- -DLOCALEDIR=\"/usr/local/share/locale\" -DPACKAGE=\"xfsprogs\"
- -I./include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -funsigned-char
- -fno-strict-aliasing -Wall -I. -g -O2 -DNDEBUG -DVERSION=\"2.9.3\"
- -DLOCALEDIR=\"/usr/local/share/locale\" -DPACKAGE=\"xfsprogs\"
- -I../include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -funsigned-char
- -fno-strict-aliasing -Wall -c xfs_bmap.c  -fPIC -DPIC -o
.libs/xfs_bmap.o

xfs_bmap.c: In function 'xfs_bmap_add_extent':
xfs_bmap.c:287: warning: implicit declaration of function
'xfs_bmap_check_leaf_extents'

and because of this, we have:

../libxfs/.libs/libxfs.a(xfs_bmap.o)(.text+0x6cf1): In function
`xfs_bmap_add_extent':
xfsprogs-2.9.3/libxfs/xfs_bmap.c:287: undefined reference to
`xfs_bmap_check_leaf_extents'
collect2: ld returned 1 exit status
gmake[1]: *** [xfs_copy] Error 1


- --
Pozdrawiam/Regards - Bogdan                         (Linux & FreeDOS)
Kurs asemblera x86  (DOS & Linux):  http://rudy.mif.pg.gda.pl/~bogdro
Grupy dyskusyjne o asm:  pl.comp.lang.asm alt.pl.asm alt.pl.asm.win32
Rozmawiaj bezpiecznie: www.JabberPL.org Surfuj anonimowo: tor.eff.org
-----BEGIN PGP SIGNATURE-----

iD8DBQFG1vBmNTrTaBxW2h4RA49wAKCTVaJ47Fp/hAVGbJd+ewVvbeoxLACeOIrL
sslZvDs9bl6QG/W09SwosYw=
=s/eu
-----END PGP SIGNATURE-----

Re: About errors in XFS header files and xfsprogs

[Christoph Hellwig]

On Thu, Aug 30, 2007 at 06:29:26PM +0200, Bogdan wrote:
>                                The header files should not only be
> ISO-C-compatible, but even ANSI-C-compatible. This will allow anyone
> to use them.

Sais who?  The GNU C extensions are very useful for programming, and
there is no reason to move to an inferior dialect.

>  Please, make useful public functions available in header files.
> Especially platform_check_ismounted() and platform_check_iswritable()
> and other "platform" functions.
> 
>  Also, anything that is a part of the public interface, should be in
> the "xfs namespace", i.e. start with "xfs_". At least one issue of
> this type is known: the list_head structure, declared in other files
> from other libraries. Things like these are very important to fix,
> because they are making other header files useless.

There is no such thing as a public inteface.  There's a good reason
libxfs is not a shared libary - it's really nothing more but a collection
of useful routines for the xfs utilities.

If you think a proper shared library with a defined interface is a good
thing you're probably right, and people would welcome you to work on one.

Re: About errors in XFS header files and xfsprogs

[Martin Schröder]

2007/8/30, Christoph Hellwig <hch@infradead.org>:
> On Thu, Aug 30, 2007 at 06:29:26PM +0200, Bogdan wrote:
> >                                The header files should not only be
> > ISO-C-compatible, but even ANSI-C-compatible. This will allow anyone
> > to use them.
>
> Sais who?  The GNU C extensions are very useful for programming, and
> there is no reason to move to an inferior dialect.

Do you want to limit xfs to gcc/Linux? Other Unices may use other ccs.

Best
   Martin

Re: About errors in XFS header files and xfsprogs

[Nathan Scott]

On Fri, 2007-08-31 at 00:39 +0200, Martin Schröder wrote:
> 
> 
> 
> 
> 2007/8/30, Christoph Hellwig <hch@infradead.org>:
> > On Thu, Aug 30, 2007 at 06:29:26PM +0200, Bogdan wrote:
> > >                                The header files should not only be
> > > ISO-C-compatible, but even ANSI-C-compatible. This will allow
> anyone
> > > to use them.
> >
> > Sais who?  The GNU C extensions are very useful for programming, and
> > there is no reason to move to an inferior dialect.
> 
> Do you want to limit xfs to gcc/Linux? Other Unices may use other ccs.
> 

These headers compile just fine on IRIX, FreeBSD and MacOSX ... the
gcc-specific pieces are in platform dependent headers already.

cheers.

--
Nathan

[PATCH] Re: About errors in XFS header files and xfsprogs

[Bogdan]

[-- Attachment #1: Type: text/plain, Size: 4577 bytes --]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: RIPEMD160

NOTE: a copy goes to the mailing list (because of the patch).

Christoph Hellwig pisze:
> On Thu, Aug 30, 2007 at 06:29:26PM +0200, Bogdan wrote:
>>                                The header files should not only be
>> ISO-C-compatible, but even ANSI-C-compatible. This will allow anyone
>> to use them.
> 
> Sais who?  The GNU C extensions are very useful for programming, and

 Sure, I like them too. But I also write programs and like them to be
available to as many, as possible.

> there is no reason to move to an inferior dialect.

 If you want xfsprogs compatible only with GCC, you're right. But GCC
is not the only compiler out there and some people using Linux/Unix
may use something else.
 So there is a reason - portability. Inserting

#ifdef __GNUC__
#endif

around GCC-specific statements isn't really that hard. I'm not asking
for compatibility with all architectures (which might require much
more work, especially with endianess, if needed). Maybe I'll do these
#defines myself in the future.
 I myself am using GCC, but others may not. How would you feel if you
had some great piece of software and had to change some 200 lines just
to compile it (because it was written, say, for MSVC or Borland with
compiler-specific stuff)?

>>  Please, make useful public functions available in header files.
>> Especially platform_check_ismounted() and platform_check_iswritable()
>> and other "platform" functions.
>>
>>  Also, anything that is a part of the public interface, should be in
>> the "xfs namespace", i.e. start with "xfs_". At least one issue of
>> this type is known: the list_head structure, declared in other files
>> from other libraries. Things like these are very important to fix,
>> because they are making other header files useless.
> 
> There is no such thing as a public inteface.

 Anything in header files that is usable for the program including
them, one might call a public interface. If libxfs.a had no public
interface, xfsprogs would not exist, because the library wouldn't have
any functions for them to call.
 If two headers define a function with the same name, we have a
conflict. Same goes for preprocessor macros. We have for example:

#define ACTIVE		1

in xfs_copy.c. If any other included header, say <pthread.h> or
<signal.h>, #defines the same name, xfs_copy.c will no longer compile.
Is this what everybody wants? Of course not. But this is exactly the
type of obstacle I've found when trying to use libxfs with libntfs
(will patch them too).

>  There's a good reason
> libxfs is not a shared libary - it's really nothing more but a collection
> of useful routines for the xfs utilities.

 Well, I'm writing a XFS utility, but its not only for XFS, hence the
conflicts. Libxfs is becoming a development library, not just a
private, internal-use bunch of routines. It is a part of a
'xfsprogs-devel' package, available probably not only for my system.
This should make you think that you aren't making the libxfs.a just
for yourselves, but also for everyone else. Thus, the library should
be designed with careful thinking, which produces less conflicts.
 Please, don't think "the user includes our file libxfs.h, so he/she
surely won't include xxx.h, yyy.h and zzz.h". Things just aren't like
this.
 You can't of course predict all header files the user will include,
and here's where inserting "xfs_" helps. You put everything in your
"namespace" using the prefix and assume no other library will use the
"xfs_" prefix. Here's where my hopes on libxfs (and partially on
libntfs, too) failed, but hopefully we will fix all the problems.

> If you think a proper shared library with a defined interface is a good
> thing you're probably right, and people would welcome you to work on one.

 Maybe one day. Now I can help at least a little. Start with applying
the attached patch to the xfsprogs development directory tree (in the
top dir, of course). This patch removes the conflicts between libxfs
and libntfs (at least those, which I've found). Any further patches
will be sent to the mailing list, unless required otherwise.

- --
Pozdrawiam/Regards - Bogdan                         (Linux & FreeDOS)
Kurs asemblera x86  (DOS & Linux):  http://rudy.mif.pg.gda.pl/~bogdro
Grupy dyskusyjne o asm:  pl.comp.lang.asm alt.pl.asm alt.pl.asm.win32
Rozmawiaj bezpiecznie: www.JabberPL.org Surfuj anonimowo: tor.eff.org

-----BEGIN PGP SIGNATURE-----

iD8DBQFG2UUANTrTaBxW2h4RA4/YAJ9xehmvenwZhLCUBybV/Qdlxj8faACgkyLG
u/VD0b2/xe8+LpopEGo2mgk=
=a7+K
-----END PGP SIGNATURE-----

[-- Attachment #2: xfs-portability-patch01.diff.gz --]
[-- Type: application/x-gzip, Size: 10185 bytes --]

[-- Attachment #3: xfs-portability-patch01.diff.gz.sig --]
[-- Type: application/pgp-signature, Size: 64 bytes --]

Re: About errors in XFS header files and xfsprogs

[Barry Naujok]

On Fri, 31 Aug 2007 02:29:26 +1000, Bogdan <bogdandr@op.pl> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: RIPEMD160
>
>  Hello.
>
>  The XFS library header files contained in xfsprogs (2.9.3-1) contain
> many errors. Run the following command on a Linux system to verify:
>
> gcc -Wall -Wextra -Wfloat-equal -Wbad-function-cast -Wsign-compare
> - -Wunreachable-code -Wpointer-arith -Wcast-qual -Wcast-align
> - -Wstrict-prototypes -Wformat-security -Wformat-nonliteral
> - -Wnested-externs -Wshadow -Wconversion -Wdeclaration-after-statement
> - -Wundef -Wpadded -Wredundant-decls -pedantic -c -o xfs_repair.o
> xfs_repair.c
>
> (you can use any file which #includes the libxfs.h header). The use of
> 'long long' is less important. The header files should not only be
> ISO-C-compatible, but even ANSI-C-compatible. This will allow anyone
> to use them.

I wouldn't call "padding struct to align..." an error either.

I'm guessing (not 100% sure) all the "warning: ISO C forbids
braced-groups within expressions" is from the endian swap calls which
eventually gets to __swabXX() #defines with {} in them in swab.h.

Related is the warning "declaration of '__x' shadows a
previous local" caused by the beXX_add() functions doing a double
__swabXX() call. Can't see an easy solution to these.

"passing argument n of 'func' with different width due to prototype"
doesn't seem to be an issue either. Could add lots of casts to hide it.

Redeclaring optind in platform_getoptreset() in linux.h could be fixed.


>  Also, a following error occurs in the library:
>
>  gcc -g -O2 -DDEBUG -DVERSION=\"2.9.3\"
> - -DLOCALEDIR=\"/usr/local/share/locale\" -DPACKAGE=\"xfsprogs\"
> - -I./include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -funsigned-char
> - -fno-strict-aliasing -Wall -I. -g -O2 -DNDEBUG -DVERSION=\"2.9.3\"
> - -DLOCALEDIR=\"/usr/local/share/locale\" -DPACKAGE=\"xfsprogs\"
> - -I../include -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -funsigned-char
> - -fno-strict-aliasing -Wall -c xfs_bmap.c  -fPIC -DPIC -o
> .libs/xfs_bmap.o
>
> xfs_bmap.c: In function 'xfs_bmap_add_extent':
> xfs_bmap.c:287: warning: implicit declaration of function
> 'xfs_bmap_check_leaf_extents'
>
> and because of this, we have:
>
> ../libxfs/.libs/libxfs.a(xfs_bmap.o)(.text+0x6cf1): In function
> `xfs_bmap_add_extent':
> xfsprogs-2.9.3/libxfs/xfs_bmap.c:287: undefined reference to
> `xfs_bmap_check_leaf_extents'
> collect2: ld returned 1 exit status
> gmake[1]: *** [xfs_copy] Error 1

It is a known issue that libxfs does not build with DEBUG defined.
In the libxfs Makefile:

# don't try linking xfs_repair with a debug libxfs.
DEBUG = -DNDEBUG

Regards,
Barry.