Dave, Let me know if the attached patch is convincing.
I think it is important to use the "structure has member" configure features sparingly. Here is a case where the best of intention was later shown to require some minor changes. I think removing the test for f_ffree and f_favail members from "configure" would help keep the intent clear-er, the code clean-er, and maybe even easier to read and understand!
Mike Slifcak wrote:
Dave Shield wrote:
This patch eliminates an incorrect logic test in ucd-snmp/disk.c
Hmmmm.... Sorry Mike, but I'm not convinced.
Just looking at the code, the check now seems to bear absolutely no relation to the code it is allegedly protecting. Can you remind me what the compile error was previously? I'm guessing that it was complaining about 'vfs.f_favail' and/or 'vfs.f_ffree'.
At the time, I was undef'ing symbols in freebsd5.h that I shouldn't have touched, there. One of the undef's prevented the module from including sys/statvfs.h, which resulted in the compiler throwing an error on "not a member" f_favail..
Those undefs were moved to where they were most useful (in host/hr_storage.c). Subsequently, the ucd-snmp/disk.o object build succeeded.
So the obvious answer would be to add a check for STRUCT_STATVFS_HAS_F_FAVAIL (which is what is actually being used) rather than STRUCT_STATFS_HAS_F_FAVAIL (which I suspect may well be an error that's never got picked up before)
By dropping the check for STRUCT_STATVFS_HAS_F_FILES completely, my suspicion is that this code block may well never be compiled on *any* O/S. (Even the ones that need it).
I've reviewed the CVS diffs. The tests originally looked _only_ at STRUCT_STATVFS_HAS_F_FILES and STRUCT_STATFS_HAS_F_FILES.
I realise that you need to move on to other things now, but if anyone else is able to investigate the effect of a new configure.in check on 'AC_CHECK_STRUCT_FOR(statvfs,f_favail)' (both on FreeBSD, and {Open,Net}BSD) that would be incredibly useful.
Dave
After staring at this last night, I realize that you had the right solution from the beginning. I'll try configure.in on FreeBSD, but I'll leave {Open,Net}BSD to the professionals that have those platforms instantiated.
Thanks again, Dave!
By the way -- Has anyone seen an agent instantiate UCD-SNMP-MIB::dskTable lately ?
On a system I'm using, /dev/da1 holds FreeBSD 5.2, /dev/da2 holds FreeBSD 4.9
Only partitions from da1 are instantiated on the agent running on FreeBSD 4.9
I've not seen dskTable on Linux, either. All tests done after applying the
FreeBSD changes that I've been hawking.
-Mike Slifcak
If there is configuration changes needed to enable dskTable, please let me know. Thanks! -Mike Slifcak
9-Jun-2004 Mike Slifcak
This patch fixes multiple problems with logic surrounding the
calculation of dskPercentNode object.
ucd-snmp/disk.c Rev 5.2 log states
"patch #600933 from Donal Diamond: dskPercentNode for FreeBSD4".
This is where STRUCT_STATFS_HAS_F_FAVAIL and
where STRUCT_STATFS_HAS_F_FFREE are introduced, in order to
distinguish FreeBSD4 "statfs" from other BSD "statvfs".
Here is a little truth table to represent the structure/member relationships.
OS structure member
FreeBSD 4.9 statfs f_ffree
FreeBSD 5.2.1 statfs f_ffree
FreeBSD 5.2.1 statvfs f_ffree, f_favail
Note that the author of patch 600933 was intending to select
the "statfs" structure for calculations. The searching of
structure for member is not necessary. The absence of "statvfs"
is proof enough to select the "statfs" calculations.
1. The code did not work for FreeBSD 5.x, as the "C" pre-processor
statements were testing structure/member relationships that
do not exist. The code was changed to not consider the
incorrect relationships.
2. The calculation is not used for any other object.
However, it was performed before every object in this handler
was to be sensed. The code was changed to calculate
only when the dskPercentNode object is to be processed.
This offers improved readability in a heavily ifdef'd code stream.
3. Several structure/member relationship tests were introduced as
part of incorporating patch #600933, for the purpose of identifying
when the "struct statfs" is being used, as it is in the FreeBSD 4.x
builds. These are used no where else, and now that they are no longer
used, they are removed from the "configure" script, the acconfig.h,
and net-snmp-config.h.in files.
Detail: Testing for f_files in struct statfs or struct statvfs works.
Testing for f_favail in struct statfs is wrong, since that structure
does not have that member. Testing for f_ffree in struct statfs works,
but it did not help the compiler choosing the correct calculation form.
In other words, we already have a test for the presence of struct statfs
or struct statvfs in the build environment. Use that test properly, and
there is no need to test additional structure member relationships.
--- net-snmp-5.1.1/agent/mibgroup/ucd-snmp/disk.c 2004-02-03 04:56:05.000000000
-0500
+++ net-snmp/agent/mibgroup/ucd-snmp/disk.c 2004-06-09 22:35:23.883912944 -0400
@@ -566,9 +566,6 @@
double totalblks, free, used, avail, availblks;
#else
static long avail;
-#if defined(STRUCT_STATVFS_HAS_F_FILES) || defined(STRUCT_STATFS_HAS_F_FILES)
- int percent_inode;
-#endif
#endif
static long long_ret;
static char errmsg[300];
@@ -651,18 +648,7 @@
iserror = (disks[disknum].minimumspace >= 0 ?
avail < disks[disknum].minimumspace :
100 - percent <= disks[disknum].minpercent) ? 1 : 0;
-#if defined(STRUCT_STATVFS_HAS_F_FILES) || defined STRUCT_STATFS_HAS_F_FAVAIL
- percent_inode = vfs.f_favail <= 0 ? 100 :
- (int) ((double) (vfs.f_files - vfs.f_ffree) /
- (double) (vfs.f_files -
- (vfs.f_ffree - vfs.f_favail)) * 100.0 + 0.5);
-#else
-#if defined(STRUCT_STATFS_HAS_F_FILES) && defined(STRUCT_STATFS_HAS_F_FFREE)
- percent_inode = vfs.f_files == 0 ? 100.0 :
- (int) ((double) (vfs.f_files - vfs.f_ffree) /
- (double) (vfs.f_files) * 100.0 + 0.5);
-#endif
-#endif /* defined(STRUCT_STATVFS_HAS_F_FILES) */
+
switch (vp->magic) {
case DISKTOTAL:
long_ret = (long)(vfs.f_blocks * multiplier);
@@ -675,9 +661,18 @@
case DISKPERCENT:
long_ret = percent;
return ((u_char *) (&long_ret));
-#if defined(STRUCT_STATVFS_HAS_F_FILES) || defined (STRUCT_STATFS_HAS_F_FILES)
+#if defined(STRUCT_STATVFS_HAS_F_FILES) || defined(STRUCT_STATFS_HAS_F_FILES)
case DISKPERCENTNODE:
- long_ret = percent_inode;
+ #if defined(STRUCT_STATVFS_HAS_F_FILES)
+ long_ret = vfs.f_favail <= 0 ? 100 :
+ (long) ((double) (vfs.f_files - vfs.f_ffree) /
+ (double) (vfs.f_files -
+ (vfs.f_ffree - vfs.f_favail)) * 100.0 + 0.5);
+ #elif defined(STRUCT_STATFS_HAS_F_FILES)
+ long_ret = vfs.f_files == 0 ? 100 :
+ (long) ((double) (vfs.f_files - vfs.f_ffree) /
+ (double) (vfs.f_files) * 100.0 + 0.5);
+ #endif
return ((u_char *) (&long_ret));
#endif
case ERRORFLAG:
--- ./configure.in 2004-06-07 22:02:50.000000000 -0400
+++ ./configure.in 2004-06-09 16:43:30.778592824 -0400
@@ -2766,38 +2766,6 @@
],statfs,f_files)
AC_CHECK_STRUCT_FOR([
-#ifdef HAVE_SYS_STAT_H
-#include <sys/stat.h>
-#endif
-#if HAVE_SYS_STATFS_H
-#include <sys/statfs.h>
-#endif
-#ifdef HAVE_SYS_PARAM_H
-#include <sys/param.h>
-#include <sys/types.h>
-#endif
-#ifdef HAVE_SYS_MOUNT_H
-#include <sys/mount.h>
-#endif
-],statfs,f_ffree)
-
-AC_CHECK_STRUCT_FOR([
-#ifdef HAVE_SYS_STAT_H
-#include <sys/stat.h>
-#endif
-#if HAVE_SYS_STATFS_H
-#include <sys/statfs.h>
-#endif
-#ifdef HAVE_SYS_PARAM_H
-#include <sys/param.h>
-#include <sys/types.h>
-#endif
-#ifdef HAVE_SYS_MOUNT_H
-#include <sys/mount.h>
-#endif
-],statfs,f_favail)
-
-AC_CHECK_STRUCT_FOR([
#if HAVE_NLIST_H
#include <nlist.h>
#endif
--- ./acconfig.h 2004-06-08 22:41:40.000000000 -0400
+++ ./acconfig.h 2004-06-09 16:46:39.778860416 -0400
@@ -267,8 +267,6 @@
/* statfs inode structure tests*/
#undef STRUCT_STATFS_HAS_F_FILES
-#undef STRUCT_STATFS_HAS_F_FFREE
-#undef STRUCT_STATFS_HAS_F_FAVAIL
/* des_ks_struct.weak_key */
#undef STRUCT_DES_KS_STRUCT_HAS_WEAK_KEY
--- ./include/net-snmp/net-snmp-config.h.in 2004-06-07 22:51:54.000000000 -0400
+++ ./include/net-snmp/net-snmp-config.h.in 2004-06-09 16:47:48.452420448 -0400
@@ -1045,8 +1045,6 @@
/* statfs inode structure tests*/
#undef STRUCT_STATFS_HAS_F_FILES
-#undef STRUCT_STATFS_HAS_F_FFREE
-#undef STRUCT_STATFS_HAS_F_FAVAIL
/* des_ks_struct.weak_key */
#undef STRUCT_DES_KS_STRUCT_HAS_WEAK_KEY
