gio/glocalfile.c correctly has logic to decide whether statfs or statvfs should be used.

Not sure about gio/gunixmounts.c:573: struct statfs is "protected" by HAVE_GETMNTINFO, HAVE_FSTAB_H and HAVE_SYS_MOUNT_H. I would add HAVE_SYS_STATFS_H, as getmintinfo() can return struct statvfs on some systems.

And then why use __INTERIX for the statvfs version? Maybe that should be elif HAVE_SYS_STATVFS?

It looks to me as though configure.ac has forgotten the possibility of not having a struct statfs.

Practically finished sorting out a patch: maintainers: I don't know the form on how you deal with "other" modules - can I assign this bug to myself and sort it out?

(PS found an issue with dtrace too while looking at configure.ac)

Created attachment 168731 [details] [review]
Fixes for platforms other than linux

 - move choice of statfs vs statvfs from gio/glocalfile.c to configure.ac,
   and use choice in gio/gunixmounts.c as well.
 - configure.ac: both dtrace and sys/sdt.h need to be present to use dtrace.
   Current test uses dtrace if just sys/sdt.h is present.
 - configure.ac: macros go in m4macros

Review of attachment 168731 [details] [review]:

Looks reasonable at first sight. I haven't tested it, though...

It looks as though my patch addresses a small part of Bug 583330, and the original poster's problem (related to Bug 133344) seems to be addressed properly by

http://cvsweb.netbsd.org/bsdweb.cgi/pkgsrc/devel/glib2/patches/patch-ak?rev=1.6&content-type=text/x-cvsweb-markup&only_with_tag=MAIN

(The patch linked in the above comment also addresses 2 other things - it's the first part of that patch which takes care of both options in comment 1)

Created attachment 192205 [details] [review]
fix statvfs vs statfs

So, since last year, two bits of this patch were rediscovered and applied:

dd9f8b8c Tue Sep 28 2010 sdt.h
33c7a9d8 Fri Jan  7 2011 m4macros

Here is a resubmission of the rest in the hope that it gets in this time...

Review of attachment 192205 [details] [review]:

Looks good to me; thank you!

It seems that this patch has broken support for OS X. I'm not really familiar with what exactly the patch does, but right now I get the following error:

gunixmounts.c: In function ‘_g_get_unix_mounts’:
gunixmounts.c:608: warning: passing argument 1 of ‘getmntinfo’ from incompatible pointer type

Which on OS X takes a statfs (not statvfs), however during configure it has been determined that statvfs should be used.

Any chance of a copy of your config.log, and maybe a copy of your /usr/include/sys/mount.h?

Created attachment 193591 [details]
config.log result of configure on OS X

The config.log

Created attachment 193592 [details]
The /usr/include/sys/mount.h

The /usr/include/sys/mount.h on OS X 10.6

Please shoot me down if I'm wrong, but it seems the test to choose between statfs and statvfs when they are both available is wrong for OS X:

Excerpt from configure.ac:

# Some systems have both statfs and statvfs, pick the most "native" for these
AS_IF([test x$ac_cv_func_statfs = xyes && test x$ac_cv_func_statvfs = xyes],
   [
   # on solaris and irix, statfs doesn't even have the f_bavail field
   AS_IF([test x$ac_cv_member_struct_statfs_f_bavail = xyes],
      [ac_cv_func_statfs=no],
   # else, at least on linux, statfs is the actual syscall
      [ac_cv_func_statvfs=no])
   ])

On OS X, f_bavail is a member of statfs

I'm afraid I don't have a handy macos box to try this out, so I'll have to ask you for rather basic information :-(

It seems that configure doesn't find sys/statfs.h on your system, as

/usr/bin/gcc-4.2 -c -O0 -ggdb3 -arch x86_64 -mmacosx-version-min=10.6 -Wall -I/Users/jesse/gtk/inst/include -DG_DISABLE_SINGLE_INCLUDES conftest.c >&5

fails (according to the config.log you sent).

You say that OS X uses statfs, so what headers would you use? (Not sys/statfs.h ?)

Sorry, I think that was a stupid question (reading your mount.h)

Indeed, statfs is defined in mount.h. The error about not finding sys/statfs.h is not very relevant (as I understand it simply tests for the availability of statfs call in some headers, including mount.h).

Noting this comment:
# on solaris and irix, statfs doesn't even have the f_bavail field

It seems like a bad heuristic to use this field to determine whether to use statfs or statvfs.

Here is what I was writing while you wrote comment 18, so I assume the
answer to "is this plausible?" is yes!:

A colleague just leant me a mac, so here is what I think is happening:
sys/mount.h defines struct statfs and declares statfs()
sys/statvfs.h defines struct statvfs and declares statvfs()
As statvfs are provided as wrappers to statfs,  the call to statvfs in
glocalfile.c should work, so the choice USE_STATVFS should be OK.

The problem is then the assumption that if statvfs() is OK, getmntinfo takes a
struct statvfs.

I'll write a test based on the examples (though obviously not testing for the
OS):

Linux:
        no getmntinfo
BSD:
        #include <sys/types.h>
        #include <sys/statvfs.h>
        int getmntinfo(struct statvfs **mntbufp, int flags);
Mac OS X:
        #include <sys/param.h>
        #include <sys/ucred.h>
        #include <sys/mount.h>
        int getmntinfo(struct statfs **mntbufp, int flags);

Sound plausible?

Created attachment 193681 [details] [review]
statfs statvfs getmntinfo fchixes pat

While writing the getmntinfo test, I fixed two other parts, I had the logic of

-# if !defined(HAVE_STRUCT_STATFS_F_BAVAIL)
-   /* on solaris and irix, statfs doesn't even have the
-      f_bavail field */
-#  define USE_STATVFS

exactly inverted, so this is the right way around:
    # on solaris and irix, statfs doesn't even have the f_bavail field
-   AS_IF([test x$ac_cv_member_struct_statfs_f_bavail = xyes],
+   AS_IF([test x$ac_cv_member_struct_statfs_f_bavail != xyes],
       [ac_cv_func_statfs=no],

I also added (typeof(statfs_buffer)=struct statvfs - don't be put off by name)
+#if defined(HAVE_STRUCT_STATVFS_F_FSTYPENAME)
+  fstype = g_strdup(statfs_buffer.f_fstypename); 
which looks the same as the statfs case, so I don't see why f_fstypename
shouldn't be used if it is available.

OK to commit?

Review of attachment 193681 [details] [review]:

I tested this on GNOME/Linux and it looks fine.

Let's see if we can get feedback from the OS X people.

::: gio/glocalfile.c
@@ +1004,3 @@
+#elif defined(USE_STATVFS)
+#if defined(HAVE_STRUCT_STATVFS_F_FSTYPENAME)
+  fstype = g_strdup(statfs_buffer.f_fstypename); 

We use a space between identifier and paren.

@@ +1008,1 @@
   fstype = g_strdup(statfs_buffer.f_basetype); 

Ditto.

John, can you try this patch?

*** Bug 655027 has been marked as a duplicate of this bug. ***

Created attachment 193718 [details]
Config.log from patch 193681

Unfortunately, no:

  CC     libgio_2_0_la-gunixfdlist.lo
  CC     libgio_2_0_la-gunixfdmessage.lo
  CC     libgio_2_0_la-gunixmount.lo
  CC     libgio_2_0_la-gunixmounts.lo
gunixmounts.c: In function '_g_get_unix_mounts':
gunixmounts.c:608: warning: passing argument 1 of 'getmntinfo' from incompatible pointer type
gunixmounts.c:617: error: 'struct statvfs' has no member named 'f_mntonname'
gunixmounts.c:618: error: 'struct statvfs' has no member named 'f_mntfromname'
gunixmounts.c:619: error: 'struct statvfs' has no member named 'f_fstypename'
gunixmounts.c:621: error: 'struct statvfs' has no member named 'f_flags'
gunixmounts.c: In function '_g_get_unix_mount_points':
gunixmounts.c:1006: warning: unused variable 'len'
make[4]: *** [libgio_2_0_la-gunixmounts.lo] Error 1

Created attachment 193720 [details]
MacOSX10.4u.sdk mount.h

The bit that's missing is that we ( the gtk-quartz maintainers) are trying to maintain backward compatibility with 10.4, and Apple changed struct statvs between 10.5 and 10.6. I'm starting a check build on 10.7, using the 10.7 SDK, but that will take a couple of hours (I have to start from bootstrap) and I'll report back when it's done. (I expect that it will succeed.)

Never mind my ranting about 10.4 compatibility: That wasn't the problem at all. 
The problem is that the getmntinfo argument check isn't working:

configure:23228: checking type of argument to getmntinfo
configure:23255: /usr/bin/gcc-4.2 -c -O0 -ggdb3 -arch i386 -I/Developer/SDKs/MacOSX10.5.sdk/usr/include -isysroot /Developer/SDKs/MacOSX10.5.sdk -mmacosx-version-min=10.5 -Wall -I/usr/local/gtk-unstable-3/inst/include -I/Developer/SDKs/MacOSX10.5.sdk/usr/include -DG_DISABLE_SINGLE_INCLUDES conftest.c
conftest.c: In function 'main':
conftest.c:148: warning: passing argument 1 of 'getmntinfo' from incompatible pointer type
configure:23255: $? = 0
configure:23256: result: struct statvfs**

The warning isn't strong enough to make the compile fail, so even though
/usr/include/sys/mount.h:int	getmntinfo(struct statfs **, int) __DARWIN_INODE64(getmntinfo);
it succeeds and sets GETMNTINFO_STRUCT_STATVFS.

One could work around that by setting -Werror in $CFLAGS, but compilation of glib isn't free of warnings, so that's not really feasible. Is there a way to force it for only that check?

Created attachment 193953 [details] [review]
force error in getmntinfo check

Sorry for wasting time trying to do "better". This version of the previous patch does exactly as you suggest and adds Werror. (Just for the test, not in general. Adding Werror then gets me an erroneous error from the uninitialised use of struct warning hence the spurious addition of =NULL to keep gcc happy.) AFAICT Werror is only correct for GCC, so I only set it in that case. I wonder what happens if you try llvm/clang on macos...

(Just realised: I haven't added the space as per comment 21 :-/ (BTW you worked out which line I cut and pasted ;-) ))

Comment on attachment 193953 [details] [review]
force error in getmntinfo check

OK, this one works.

I haven't tried building Gtk with clang yet, but I did look in the documentation. It uses -Werror just like gcc, so this should be OK there, too.

I would suggest applying this patch (plus the missing space) - it fixes the problem case under consideration, which is building on macos with gcc. Colin?

I know that -Werror is necessary for, and works on GCC, and the test for GCC is easy.
It seems that -Werror is also OK for clang and pcc, but is it necessary?
I just tried cc: Sun Ceres C 5.10 SunOS_i386 2009/03/06 and it didn't understand -Werror.
It may be that other compiles don't need an extra flag to make that warning an error.
Either way, I would rather complicate if we find it necessary, but not otherwise...

Created attachment 194308 [details] [review]
Patrick's patch with already-fixed section removed

OK, that works now, except that Mattias checked in http://git.gnome.org/browse/glib/commit/configure.ac?id=3c504e47656e7d26c0c24465f492b92f673a5cec which already does one of your changes and makes the patch not apply. This fixes that little problem so that it applies against master.

I also fixed the spaces before opening parens, though I see another violation in one of the context lines. Whoever commits can fix that up. ;-)

Created attachment 194313 [details] [review]
Patrick's patch with already-fixed section removed and spaces-before-open-parens fixed

Darn it, didn't save the spaces-fixed patch before submitting. This one is right.

Review of attachment 194313 [details] [review]:

I know the details were discussed here in the bug, but I'd like to at least see some attempt to summarize the changes in the commit message.   Following https://live.gnome.org/GnomeLove/SubmittingPatches the commit body must *at least* have a link to this bug.

The point is that someone looking 5 years later for *why* things changed will be able to find out - "statfs/statvfs fixes" doesn't cover it.

::: configure.ac
@@ +1010,3 @@
 # successfully compiles, but warns to use the newer statvfs interface:
 AS_IF([test $ac_cv_header_sys_statvfs_h = yes], [AC_CHECK_FUNCS([statvfs])])
+AS_IF([test $ac_cv_header_sys_statfs_h  = yes -o $ac_cv_header_sys_mount_h = yes], [AC_CHECK_FUNCS([statfs])])

Missing shell empty string hardening here.  You need to either include $ac_cv_header_sys_statfs_h in double quotes or add an x prefix.  Search for "xyes".

(Autoconf may be guaranteed to set it, but it's better to be consistent in our use of "test".

@@ +1157,3 @@
+# Check whether getmntinfo takes struct statfs or struct statvfs as an argument
+#
+AS_IF([test $ac_cv_func_getmntinfo = yes],

Ditto here.

@@ +1159,3 @@
+AS_IF([test $ac_cv_func_getmntinfo = yes],
+[AC_MSG_CHECKING([type of argument to getmntinfo])
+ AS_IF([test x$GCC = xyes],

Note this one is safe with the "x" prefix.

@@ +1163,3 @@
+        CFLAGS="-Werror $CFLAGS"])
+ AC_COMPILE_IFELSE(
+	[AC_LANG_PROGRAM(

This is kind of hard to read - you could split it up into separate sections, but I'm ok with how it is now too.

By split it up I mean:

AC_COMPILE_IFELSE(firstprog, have_foo=yes, have_foo=no)
if test x$have_foo = xno; then
  AC_COMPILE_IFELSE(...)
fi

(In reply to comment #19)
> BSD:
>         int getmntinfo(struct statvfs **mntbufp, int flags);
> Mac OS X:
>         int getmntinfo(struct statfs **mntbufp, int flags);

actually, apparently it's only NetBSD that uses statvfs.

Also, getmntinfo() is apparently just a wrapper around getfsstat() (or, on NetBSD, getvfsstat()) that leaks memory and fails to be threadsafe, for your convenience.

So rather than relying on -Werror to work to get the right result, a better solution would be:

    - test for the existence of getfsstat and getvfsstat instead of
      getmntinfo

    - Change _g_get_unix_mounts() to call getfsstat()/getvfsstat() with
      NULL, allocate an array of statfs/statvfs, call
      getfsstat()/getvfsstat() again with the right bufsize, process
      the results, and then free the array.

Created attachment 195388 [details] [review]
use getstat{v,}fsstat for which problematic getmntinfo is a wrapper

Implement Dan Winship's good idea to use functions whose names differ, rather than a function which can take varying parameter types.

Created attachment 195389 [details] [review]
harrden configure string tests

Style change: essentially

if test $ac_cv_var = yes   ----> if test x$ac_cv_var = xyes

throughout.

(I'm ambivalent about it myself: as noted in comment 33, I "hardened" the test for GCC above, as I thought it was set to yes if found, but may have been empty otherwise. I didn't "harden" the cached variables as, as also noted in comment 33, they are guaranteed to be set.)

These fail for me on Linux/glibc:

| /src/glib/gio/gunixmounts.c:191:23: fatal error: sys/ucred.h: No such file or directory

(In reply to comment #37)
> These fail for me on Linux/glibc:
> 
> | /src/glib/gio/gunixmounts.c:191:23: fatal error: sys/ucred.h: No such file or
> directory

-#if defined(HAVE_GETMNTINFO) && defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)
+#if defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)

needs to be something like

#if (defined(HAVE_GETVFSSTAT) || defined(HAVE_GETFSSTAT)) && defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)

(it compiles on Linux with that)

Created attachment 195512 [details] [review]
use getstat{v,}fsstat for which problematic getmntinfo is a wrapper

Use

#if (defined(HAVE_GETVFSSTAT) || defined(HAVE_GETFSSTAT)) && defined(HAVE_FSTAB_H) && defined(HAVE_SYS_MOUNT_H)

everywhere HAVE_GETMNTINFO appeared. Essentially the above combination of defines becomes a flag for the way it is done on a particular system. I suppose automake conditionals are looking appealing...

(The 2nd patch ought to apply after this, and I tested it on NetBSD and ubuntu 10.04)

committed with a few minor fixes (in particular, the getfsstat()
systems still use MNT_NOWAIT, not ST_NOWAIT). Thanks for the patch.