Libguestfs - Oct 2017 - [PATCH 0/2] builder: Choose better weights in the planner.

It started out as "this'll be just a simple fix ..."
and turned into something a bit over-engineered in the end.

Here it is anyway.

Rich.

common/mlutils: Unix_utils.StatVFS.statvfs: This commit implements a
full-featured binding for the statvfs(3) function which also includes
the statfs(2) f_type field.

We then use this to reimplement the daemon statvfs API in OCaml.

Note that the Gnulib fallback is fixed in this commit.  It previously
referenced non-existent field names in the fs_usage struct.
---
 common/mlutils/unix_utils-c.c | 171 ++++++++++++++++++++++++++++++++++++---
 common/mlutils/unix_utils.ml  |  22 ++++-
 common/mlutils/unix_utils.mli |  26 +++++-
 daemon/Makefile.am            |  11 ++-
 daemon/statvfs.c              | 184 ------------------------------------------
 daemon/statvfs.ml             |  43 ++++++++++
 daemon/statvfs.mli            |  33 ++++++++
 docs/C_SOURCE_FILES           |   1 -
 generator/actions_core.ml     |   1 +
 m4/guestfs_libraries.m4       |   5 +-
 po/POTFILES                   |   1 -
 sparsify/copying.ml           |   2 +-
 v2v/v2v.ml                    |   2 +-
 13 files changed, 294 insertions(+), 208 deletions(-)

diff --git a/common/mlutils/unix_utils-c.c b/common/mlutils/unix_utils-c.c
index 631f1fddd..95501ba22 100644
--- a/common/mlutils/unix_utils-c.c
+++ b/common/mlutils/unix_utils-c.c
@@ -27,7 +27,18 @@
 #include <fnmatch.h>
 #include <errno.h>
 #include <sys/types.h>
+
+#ifdef HAVE_SYS_STATFS_H
+#include <sys/statfs.h>
+#endif
+
+#ifdef HAVE_SYS_STATVFS_H
 #include <sys/statvfs.h>
+#endif
+
+#ifdef HAVE_SYS_VFS_H
+#include <sys/vfs.h>
+#endif
 
 #if MAJOR_IN_MKDEV
 #include <sys/mkdev.h>
@@ -36,12 +47,20 @@
 /* else it's in sys/types.h, included above */
 #endif
 
+#ifdef HAVE_WINDOWS_H
+#include <windows.h>
+#endif
+
 #include <caml/alloc.h>
 #include <caml/fail.h>
 #include <caml/memory.h>
 #include <caml/mlvalues.h>
 #include <caml/unixsupport.h>
 
+#include "fsusage.h"
+
+#include "cleanups.h"
+
 extern value guestfs_int_mllib_dev_t_makedev (value majv, value minv);
 extern value guestfs_int_mllib_dev_t_major (value devv);
 extern value guestfs_int_mllib_dev_t_minor (value devv);
@@ -52,7 +71,7 @@ extern value guestfs_int_mllib_sync (value unitv);
 extern value guestfs_int_mllib_fsync_file (value filenamev);
 extern value guestfs_int_mllib_mkdtemp (value val_pattern);
 extern value guestfs_int_mllib_realpath (value pathv);
-extern value guestfs_int_mllib_statvfs_free_space (value pathv);
+extern value guestfs_int_mllib_statvfs_statvfs (value pathv);
 
 /* NB: This is a "noalloc" call. */
 value
@@ -207,20 +226,150 @@ guestfs_int_mllib_realpath (value pathv)
 }
 
 value
-guestfs_int_mllib_statvfs_free_space (value pathv)
+guestfs_int_mllib_statvfs_statvfs (value pathv)
 {
   CAMLparam1 (pathv);
-  CAMLlocal1 (rv);
-  struct statvfs buf;
-  int64_t free_space;
+  int64_t f_bsize;
+  int64_t f_frsize;
+  int64_t f_blocks;
+  int64_t f_bfree;
+  int64_t f_bavail;
+  int64_t f_files;
+  int64_t f_ffree;
+  int64_t f_favail;
+  int64_t f_fsid;
+  int64_t f_flag;
+  int64_t f_namemax;
+  int64_t f_type;
+  CAMLlocal2 (rv, v);
 
-  if (statvfs (String_val (pathv), &buf) == -1) {
-    perror ("statvfs");
-    caml_failwith ("statvfs");
-  }
+#ifdef HAVE_FSTATVFS
+  CLEANUP_CLOSE int fd = -1;
+  struct statvfs buf1;
+#ifdef HAVE_FSTATFS
+  struct statfs buf2;
+#endif
 
-  free_space = (int64_t) buf.f_bsize * buf.f_bavail;
-  rv = caml_copy_int64 (free_space);
+  fd = open (String_val (pathv), O_RDONLY);
+  if (fd == -1)
+    unix_error (errno, (char *) "statvfs: open", pathv);
+  if (fstatvfs (fd, &buf1) == -1)
+    unix_error (errno, (char *) "statvfs: statvfs", pathv);
+#ifdef HAVE_FSTATFS
+  if (fstatfs (fd, &buf2) == -1)
+    unix_error (errno, (char *) "statvfs: statfs", pathv);
+#endif
+
+  f_bsize = buf1.f_bsize;
+  f_frsize = buf1.f_frsize;
+  f_blocks = buf1.f_blocks;
+  f_bfree = buf1.f_bfree;
+  f_bavail = buf1.f_bavail;
+  f_files = buf1.f_files;
+  f_ffree = buf1.f_ffree;
+  f_favail = buf1.f_favail;
+  f_fsid = buf1.f_fsid;
+  f_flag = buf1.f_flag;
+  f_namemax = buf1.f_namemax;
+#ifdef HAVE_FSTATFS
+  f_type = buf2.f_type;
+#else
+  f_type = -1;
+#endif
+
+#else /* !HAVE_FSTATVFS */
+#  if WIN32
+  ULONGLONG free_bytes_available; /* for user - similar to bavail */
+  ULONGLONG total_number_of_bytes;
+  ULONGLONG total_number_of_free_bytes; /* for everyone - bfree */
+
+  if (!GetDiskFreeSpaceEx (String_val (pathv),
+                           (PULARGE_INTEGER) &free_bytes_available,
+                           (PULARGE_INTEGER) &total_number_of_bytes,
+                           (PULARGE_INTEGER) &total_number_of_free_bytes))
+    unix_error (EIO, (char *) "statvfs: GetDiskFreeSpaceEx", pathv);
+
+  /* XXX I couldn't determine how to get block size.  MSDN has a
+   * unhelpful hard-coded list here:
+   *   http://support.microsoft.com/kb/140365
+   * but this depends on the filesystem type, the size of the disk and
+   * the version of Windows.  So this code assumes the disk is NTFS
+   * and the version of Windows is >= Win2K.
+   */
+  if (total_number_of_bytes < UINT64_C (16) * 1024 * 1024 * 1024 * 1024)
+    f_bsize = 4096;
+  else if (total_number_of_bytes < UINT64_C (32) * 1024 * 1024 * 1024 *
1024)
+    f_bsize = 8192;
+  else if (total_number_of_bytes < UINT64_C (64) * 1024 * 1024 * 1024 *
1024)
+    f_bsize = 16384;
+  else if (total_number_of_bytes < UINT64_C (128) * 1024 * 1024 * 1024 *
1024)
+    f_bsize = 32768;
+  else
+    f_bsize = 65536;
+
+  /* As with stat, -1 indicates a field is not known. */
+  f_frsize = ret->bsize;
+  f_blocks = total_number_of_bytes / ret->bsize;
+  f_bfree = total_number_of_free_bytes / ret->bsize;
+  f_bavail = free_bytes_available / ret->bsize;
+  f_files = -1;
+  f_ffree = -1;
+  f_favail = -1;
+  f_fsid = -1;
+  f_flag = -1;
+  f_namemax = FILENAME_MAX;
+  f_type = -1;
+
+#  else /* !WIN32 */
+  /* This replacement function is provided by Gnulib. */
+  struct fs_usage fsu;
+
+  if (get_fs_usage (String_val (pathv), String_val (pathv), &fsu) == -1)
+    unix_error (EIO, (char *) "statvfs: get_fs_usage", pathv);
+
+  /* As with stat, -1 indicates a field is not known. */
+  f_bsize = fsu.fsu_bsize;
+  f_frsize = -1;
+  f_blocks = fsu.fsu_blocks;
+  f_bfree = fsu.fsu_bfree;
+  f_bavail = fsu.fsu_bavail;
+  f_files = fsu.fsu_files;
+  f_free = fsu.fsu_ffree;
+  f_favail = -1;
+  f_fsid = -1;
+  f_flag = -1;
+  f_namemax = -1;
+  f_type = -1;
+
+#  endif /* !WIN32 */
+#endif /* !HAVE_FSTATVFS */
+
+  /* Construct the return struct. */
+  rv = caml_alloc (12, 0);
+  v = caml_copy_int64 (f_bsize);
+  Store_field (rv, 0, v);
+  v = caml_copy_int64 (f_frsize);
+  Store_field (rv, 1, v);
+  v = caml_copy_int64 (f_blocks);
+  Store_field (rv, 2, v);
+  v = caml_copy_int64 (f_bfree);
+  Store_field (rv, 3, v);
+  v = caml_copy_int64 (f_bavail);
+  Store_field (rv, 4, v);
+  v = caml_copy_int64 (f_files);
+  Store_field (rv, 5, v);
+  v = caml_copy_int64 (f_ffree);
+  Store_field (rv, 6, v);
+  v = caml_copy_int64 (f_favail);
+  Store_field (rv, 7, v);
+  v = caml_copy_int64 (f_fsid);
+  Store_field (rv, 8, v);
+  v = caml_copy_int64 (f_flag);
+  Store_field (rv, 9, v);
+  v = caml_copy_int64 (f_namemax);
+  Store_field (rv, 10, v);
+  v = caml_copy_int64 (f_type);
+  Store_field (rv, 11, v);
 
   CAMLreturn (rv);
 }
diff --git a/common/mlutils/unix_utils.ml b/common/mlutils/unix_utils.ml
index b135bcaee..3eb3ac890 100644
--- a/common/mlutils/unix_utils.ml
+++ b/common/mlutils/unix_utils.ml
@@ -16,6 +16,8 @@
  * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
  *)
 
+open Std_utils
+
 module Dev_t = struct
   external makedev : int -> int -> int =
"guestfs_int_mllib_dev_t_makedev" "noalloc"
   external major : int -> int = "guestfs_int_mllib_dev_t_major"
"noalloc"
@@ -61,6 +63,22 @@ module Realpath = struct
 end
 
 module StatVFS = struct
-  external free_space : string -> int64 -   
"guestfs_int_mllib_statvfs_free_space"
+  type statvfs = {
+    f_bsize : int64;
+    f_frsize : int64;
+    f_blocks : int64;
+    f_bfree : int64;
+    f_bavail : int64;
+    f_files : int64;
+    f_ffree : int64;
+    f_favail : int64;
+    f_fsid : int64;
+    f_flag : int64;
+    f_namemax : int64;
+    f_type : int64;
+  }
+  external statvfs : string -> statvfs +   
"guestfs_int_mllib_statvfs_statvfs"
+
+  let free_space { f_bsize = bsize; f_bavail = bavail } = bsize *^ bavail
 end
diff --git a/common/mlutils/unix_utils.mli b/common/mlutils/unix_utils.mli
index f4f8ca578..385791cac 100644
--- a/common/mlutils/unix_utils.mli
+++ b/common/mlutils/unix_utils.mli
@@ -94,7 +94,29 @@ module Realpath : sig
 end
 
 module StatVFS : sig
-  val free_space : string -> int64
-  (** [free_space path] returns the free space available on the
+  type statvfs = {
+    f_bsize : int64;            (** Filesystem block size *)
+    f_frsize : int64;           (** Fragment size *)
+    f_blocks : int64;           (** Size of fs in f_frsize units *)
+    f_bfree : int64;            (** Number of free blocks *)
+    f_bavail : int64;           (** Number of free blocks for non-root *)
+    f_files : int64;            (** Number of inodes *)
+    f_ffree : int64;            (** Number of free inodes *)
+    f_favail : int64;           (** Number of free inodes for non-root *)
+    f_fsid : int64;             (** Filesystem ID *)
+    f_flag : int64;             (** Mount flags *)
+    f_namemax : int64;          (** Maximum length of filenames *)
+    f_type : int64;             (** Type of filesystem (see statfs(2)) *)
+  }
+  val statvfs : string -> statvfs
+  (** This is a combination of [statvfs(3)] and one extra field from
+      [statfs(2)].  The main fields are returned by calling [statvfs(3)]
+      on the given path.  The [f_type] field is returned from [statfs(2)].
+
+      In case of non-Linux or non-POSIX, this call is emulated as best
+      we can with missing fields returned as [-1]. *)
+
+  val free_space : statvfs -> int64
+  (** [free_space (statvfs path)] returns the free space available on the
       filesystem that contains [path], in bytes. *)
 end
diff --git a/daemon/Makefile.am b/daemon/Makefile.am
index 61cfb5caa..070fe4ddc 100644
--- a/daemon/Makefile.am
+++ b/daemon/Makefile.am
@@ -154,7 +154,6 @@ guestfsd_SOURCES = \
 	sleuthkit.c \
 	squashfs.c \
 	stat.c \
-	statvfs.c \
 	strings.c \
 	structs-cleanups.c \
 	structs-cleanups.h \
@@ -274,6 +273,7 @@ SOURCES_MLI = \
 	optgroups.mli \
 	parted.mli \
 	realpath.mli \
+	statvfs.mli \
 	structs.mli \
 	sysroot.mli \
 	utils.mli
@@ -300,6 +300,7 @@ SOURCES_ML = \
 	parted.ml \
 	listfs.ml \
 	realpath.ml \
+	statvfs.ml \
 	inspect_types.ml \
 	inspect_utils.ml \
 	inspect_fs_unix_fstab.ml \
@@ -348,8 +349,10 @@ camldaemon.o: $(OBJECTS)
 	$(OCAMLFIND) $(BEST) -output-obj -o $@ \
 	    $(OCAMLFLAGS) $(OCAMLPACKAGES) \
 	    -linkpkg \
-	    mlaugeas.$(MLARCHIVE) mlpcre.$(MLARCHIVE) \
-	    mlcutils.$(MLARCHIVE) mlstdutils.$(MLARCHIVE) \
+	    mlaugeas.$(MLARCHIVE) \
+	    mlpcre.$(MLARCHIVE) \
+	    mlstdutils.$(MLARCHIVE) \
+	    mlcutils.$(MLARCHIVE) \
 	    $(OBJECTS)
 
 # OCaml dependencies.
@@ -399,8 +402,8 @@ endif
 
 OCAMLLINKFLAGS = \
 	mlpcre.$(MLARCHIVE) \
-	mlcutils.$(MLARCHIVE) \
 	mlstdutils.$(MLARCHIVE) \
+	mlcutils.$(MLARCHIVE) \
 	mlaugeas.$(MLARCHIVE) \
 	$(LINK_CUSTOM_OCAMLC_ONLY)
 
diff --git a/daemon/statvfs.c b/daemon/statvfs.c
deleted file mode 100644
index 987537d1f..000000000
--- a/daemon/statvfs.c
+++ /dev/null
@@ -1,184 +0,0 @@
-/* libguestfs - the guestfsd daemon
- * Copyright (C) 2009 Red Hat Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301
USA.
- */
-
-#include <config.h>
-
-#ifdef HAVE_WINDOWS_H
-#include <windows.h>
-#endif
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <sys/types.h>
-#include <fcntl.h>
-#include <unistd.h>
-#include <stdint.h>
-
-#ifdef HAVE_SYS_STATVFS_H
-#include <sys/statvfs.h>
-#endif
-
-#include <fsusage.h>
-
-#include "guestfs_protocol.h"
-#include "daemon.h"
-#include "actions.h"
-
-guestfs_int_statvfs *
-do_statvfs (const char *path)
-{
-#ifdef HAVE_STATVFS
-  int r;
-  guestfs_int_statvfs *ret;
-  struct statvfs statbuf;
-
-  CHROOT_IN;
-  r = statvfs (path, &statbuf);
-  CHROOT_OUT;
-
-  if (r == -1) {
-    reply_with_perror ("statvfs");
-    return NULL;
-  }
-
-  ret = malloc (sizeof *ret);
-  if (ret == NULL) {
-    reply_with_perror ("malloc");
-    return NULL;
-  }
-
-  ret->bsize = statbuf.f_bsize;
-  ret->frsize = statbuf.f_frsize;
-  ret->blocks = statbuf.f_blocks;
-  ret->bfree = statbuf.f_bfree;
-  ret->bavail = statbuf.f_bavail;
-  ret->files = statbuf.f_files;
-  ret->ffree = statbuf.f_ffree;
-  ret->favail = statbuf.f_favail;
-  ret->fsid = statbuf.f_fsid;
-  ret->flag = statbuf.f_flag;
-  ret->namemax = statbuf.f_namemax;
-
-  return ret;
-
-#else /* !HAVE_STATVFS */
-#  if WIN32
-
-  CLEANUP_FREE char *disk = NULL;
-  guestfs_int_statvfs *ret;
-  ULONGLONG free_bytes_available; /* for user - similar to bavail */
-  ULONGLONG total_number_of_bytes;
-  ULONGLONG total_number_of_free_bytes; /* for everyone - bfree */
-
-  disk = sysroot_path (path);
-  if (!disk) {
-    reply_with_perror ("malloc");
-    return NULL;
-  }
-
-  if (!GetDiskFreeSpaceEx (disk,
-                           (PULARGE_INTEGER) &free_bytes_available,
-                           (PULARGE_INTEGER) &total_number_of_bytes,
-                           (PULARGE_INTEGER) &total_number_of_free_bytes))
{
-    reply_with_perror ("GetDiskFreeSpaceEx");
-    return NULL;
-  }
-
-  ret = malloc (sizeof *ret);
-  if (ret == NULL) {
-    reply_with_perror ("malloc");
-    return NULL;
-  }
-
-  /* XXX I couldn't determine how to get block size.  MSDN has a
-   * unhelpful hard-coded list here:
-   *   http://support.microsoft.com/kb/140365
-   * but this depends on the filesystem type, the size of the disk and
-   * the version of Windows.  So this code assumes the disk is NTFS
-   * and the version of Windows is >= Win2K.
-   */
-  if (total_number_of_bytes < UINT64_C (16) * 1024 * 1024 * 1024 * 1024)
-    ret->bsize = 4096;
-  else if (total_number_of_bytes < UINT64_C (32) * 1024 * 1024 * 1024 *
1024)
-    ret->bsize = 8192;
-  else if (total_number_of_bytes < UINT64_C (64) * 1024 * 1024 * 1024 *
1024)
-    ret->bsize = 16384;
-  else if (total_number_of_bytes < UINT64_C (128) * 1024 * 1024 * 1024 *
1024)
-    ret->bsize = 32768;
-  else
-    ret->bsize = 65536;
-
-  /* As with stat, -1 indicates a field is not known. */
-  ret->frsize = ret->bsize;
-  ret->blocks = total_number_of_bytes / ret->bsize;
-  ret->bfree = total_number_of_free_bytes / ret->bsize;
-  ret->bavail = free_bytes_available / ret->bsize;
-  ret->files = -1;
-  ret->ffree = -1;
-  ret->favail = -1;
-  ret->fsid = -1;
-  ret->flag = -1;
-  ret->namemax = FILENAME_MAX;
-
-  return ret;
-
-#  else /* !WIN32 */
-
-  CLEANUP_FREE char *disk = NULL;
-  int r;
-  guestfs_int_statvfs *ret;
-  struct fs_usage fsu;
-
-  disk = sysroot_path (path);
-  if (!disk) {
-    reply_with_perror ("malloc");
-    return NULL;
-  }
-
-  r = get_fs_usage (disk, disk, &fsu);
-
-  if (r == -1) {
-    reply_with_perror ("get_fs_usage: %s", path);
-    return NULL;
-  }
-
-  ret = malloc (sizeof *ret);
-  if (ret == NULL) {
-    reply_with_perror ("malloc");
-    return NULL;
-  }
-
-  /* As with stat, -1 indicates a field is not known. */
-  ret->bsize = fsu.f_bsize;
-  ret->frsize = -1;
-  ret->blocks = fsu.f_blocks;
-  ret->bfree = fsu.f_bfree;
-  ret->bavail = fsu.f_bavail;
-  ret->files = fsu.f_files;
-  ret->ffree = fsu.f_ffree;
-  ret->favail = -1;
-  ret->fsid = -1;
-  ret->flag = -1;
-  ret->namemax = -1;
-
-  return ret;
-
-#  endif /* !WIN32 */
-#endif /* !HAVE_STATVFS */
-}
diff --git a/daemon/statvfs.ml b/daemon/statvfs.ml
new file mode 100644
index 000000000..9e1ad7ec9
--- /dev/null
+++ b/daemon/statvfs.ml
@@ -0,0 +1,43 @@
+(* guestfs-inspection
+ * Copyright (C) 2009-2017 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+open Unix_utils
+
+include Structs
+
+let statvfs path +  let chroot = Chroot.create ~name:(Printf.sprintf
"statvfs: %s" path) () in
+  let r = Chroot.f chroot Unix_utils.StatVFS.statvfs path in
+
+  (* [r : Unix_utils.StatVFS.statvfs] is superficially similar to
+   * the guestfs [statvfs] structure, but not the same.  We have to
+   * copy the fields.
+   *)
+  {
+    bsize = r.f_bsize;
+    frsize = r.f_frsize;
+    blocks = r.f_blocks;
+    bfree = r.f_bfree;
+    bavail = r.f_bavail;
+    files = r.f_files;
+    ffree = r.f_ffree;
+    favail = r.f_favail;
+    fsid = r.f_fsid;
+    flag = r.f_flag;
+    namemax = r.f_namemax;
+  }
diff --git a/daemon/statvfs.mli b/daemon/statvfs.mli
new file mode 100644
index 000000000..163ea9a15
--- /dev/null
+++ b/daemon/statvfs.mli
@@ -0,0 +1,33 @@
+(* guestfs-inspection
+ * Copyright (C) 2009-2017 Red Hat Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *)
+
+type statvfs = {
+  bsize : int64;
+  frsize : int64;
+  blocks : int64;
+  bfree : int64;
+  bavail : int64;
+  files : int64;
+  ffree : int64;
+  favail : int64;
+  fsid : int64;
+  flag : int64;
+  namemax : int64;
+}
+
+val statvfs : string -> statvfs
diff --git a/docs/C_SOURCE_FILES b/docs/C_SOURCE_FILES
index 41bb6c9eb..f3ee61e0d 100644
--- a/docs/C_SOURCE_FILES
+++ b/docs/C_SOURCE_FILES
@@ -166,7 +166,6 @@ daemon/sleep.c
 daemon/sleuthkit.c
 daemon/squashfs.c
 daemon/stat.c
-daemon/statvfs.c
 daemon/strings.c
 daemon/structs-cleanups.c
 daemon/structs-cleanups.h
diff --git a/generator/actions_core.ml b/generator/actions_core.ml
index 81cb99ea8..604620673 100644
--- a/generator/actions_core.ml
+++ b/generator/actions_core.ml
@@ -2319,6 +2319,7 @@ See also: C<guestfs_sh_lines>" };
   { defaults with
     name = "statvfs"; added = (1, 9, 2);
     style = RStruct ("statbuf", "statvfs"), [String
(Pathname, "path")], [];
+    impl = OCaml "Statvfs.statvfs";
     tests = [
       InitISOFS, Always, TestResult (
         [["statvfs"; "/"]], "ret->namemax ==
255"), []
diff --git a/m4/guestfs_libraries.m4 b/m4/guestfs_libraries.m4
index b8fa1446a..a3b730572 100644
--- a/m4/guestfs_libraries.m4
+++ b/m4/guestfs_libraries.m4
@@ -48,10 +48,12 @@ AC_CHECK_HEADERS([\
     sys/inotify.h \
     sys/resource.h \
     sys/socket.h \
+    sys/statfs.h \
     sys/statvfs.h \
     sys/time.h \
     sys/types.h \
     sys/un.h \
+    sys/vfs.h \
     sys/wait.h \
     windows.h \
     sys/xattr.h])
@@ -59,6 +61,8 @@ AC_CHECK_HEADERS([\
 dnl Functions.
 AC_CHECK_FUNCS([\
     be32toh \
+    fstatfs \
+    fstatvfs \
     fsync \
     futimens \
     getxattr \
@@ -80,7 +84,6 @@ AC_CHECK_FUNCS([\
     setrlimit \
     setxattr \
     sigaction \
-    statvfs \
     sync])
 
 dnl Which header file defines major, minor, makedev.
diff --git a/po/POTFILES b/po/POTFILES
index 40c0708a0..a049d66fe 100644
--- a/po/POTFILES
+++ b/po/POTFILES
@@ -149,7 +149,6 @@ daemon/sleep.c
 daemon/sleuthkit.c
 daemon/squashfs.c
 daemon/stat.c
-daemon/statvfs.c
 daemon/strings.c
 daemon/structs-cleanups.c
 daemon/stubs-0.c
diff --git a/sparsify/copying.ml b/sparsify/copying.ml
index 7d004b550..0c0b27c94 100644
--- a/sparsify/copying.ml
+++ b/sparsify/copying.ml
@@ -99,7 +99,7 @@ let run indisk outdisk check_tmpdir compress convert
           virtual_size (human_size virtual_size);
 
     let print_warning () -      let free_space = StatVFS.free_space tmpdir in
+      let free_space = StatVFS.free_space (StatVFS.statvfs tmpdir) in
       let extra_needed = virtual_size -^ free_space in
       if extra_needed > 0L then (
         warning (f_"\
diff --git a/v2v/v2v.ml b/v2v/v2v.ml
index 74ba66a3d..f54838e5e 100644
--- a/v2v/v2v.ml
+++ b/v2v/v2v.ml
@@ -272,7 +272,7 @@ and overlay_dir = (open_guestfs ())#get_cachedir ()
  * guestfs appliance which is also stored here.
  *)
 and check_host_free_space () -  let free_space = StatVFS.free_space overlay_dir
in
+  let free_space = StatVFS.free_space (StatVFS.statvfs overlay_dir) in
   debug "check_host_free_space: overlay_dir=%s free_space=%Ld"
         overlay_dir free_space;
   if free_space < 1_073_741_824L then
-- 
2.13.2

---
 builder/builder.ml            | 89 ++++++++++++++++++++++++++++++++++---------
 common/mlutils/unix_utils.ml  |  4 ++
 common/mlutils/unix_utils.mli |  6 +++
 3 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/builder/builder.ml b/builder/builder.ml
index d8e625f68..5499a4b10 100644
--- a/builder/builder.ml
+++ b/builder/builder.ml
@@ -414,12 +414,64 @@ let main ()      let is_not t = not (is t) in
     let remove = List.remove_assoc in
     let ret = ref [] in
-    let tr task weight otags = push_front (task, weight, otags) ret in
 
-    (* XXX Weights are not very smartly chosen.  At the moment I'm
-     * using a range [0..100] where 0 = free and 100 = expensive.  We
-     * could estimate weights better by looking at file sizes.
+    (* The scheme for weights ranges from 0 = free to 100 = most expensive:
+     *    0 = free operations like renaming a file in the same directory
+     *   10 = in-place conversions (like [qemu-img resize])
+     *   20 = copy or move a file between two local filesystems
+     *   30 = copy and convert a file between two local filesystems
+     *   40 = copy a file within the same local filesystem
+     *   50 = copy and convert a file within the same local filesystem
+     *   80 = copy, move, convert if source or target is on remote filesystem
+     *  100 = complex operations like virt-resize
+     * We could estimate weights better by looking at file sizes.
      *)
+    let weight task otags +      let infile = List.assoc `Filename itags
+      and outfile = List.assoc `Filename otags in
+
+      (* Get the lstat and statvfs of a file.  If the file doesn't
+       * exist, get them from the containing directory.
+       *)
+      let stats path +        let path +          if Sys.file_exists path then
path else Filename.dirname path in
+        lstat path, StatVFS.statvfs path
+      (* When calculating weight, is the filesystem remote? *)
+      and is_remote (_, { StatVFS.f_type = ft }) +        ft =
StatVFS.cifs_magic_number || ft = StatVFS.nfs_super_magic
+        || ft = StatVFS.smb_super_magic
+      (* When calculating weight, are two files on the same filesystem? *)
+      and same_filesystem ({st_dev = d1}, _) ({st_dev = d2}, _) = d1 = d2
+      in
+
+      match task with
+      | `Virt_resize -> 100     (* virt-resize is a special case*)
+      | (`Copy|`Move|`Pxzcat|`Disk_resize|`Convert) as task ->
+         let inst = stats infile in
+         let outst = stats outfile in
+         if is_remote inst || is_remote outst then 80 (* NFS etc. *)
+         else (
+           (* Copies and moves across different local filesystems.  The
+            * theory is this is less expensive than moving within
+            * filesystems because less bandwith is available (except in
+            * the special case of moving within a filesystem which is
+            * free).
+            *)
+           let across = not (same_filesystem inst outst) in
+           match task, across with
+           | `Move, false -> 0     (* rename in same filesystem *)
+           | `Disk_resize, _ -> 10 (* in-place conversion *)
+           | `Move, true -> 20     (* move between two filesystems *)
+           | `Copy, true -> 20     (* copy between two filesystems *)
+           | (`Pxzcat|`Convert), true -> 30 (* convert between two local
fses*)
+           | `Copy, false -> 40    (* copy within same filesystem *)
+           | (`Pxzcat|`Convert), false -> 50 (* convert with same local fs*)
+         )
+    in
+
+    (* Add a transition to the returned list. *)
+    let tr task otags = push_front (task, weight task otags, otags) ret in
 
     (* Since the final plan won't run in parallel, we don't only need
      * to choose unique tempfiles per transition, so this is OK:
@@ -431,17 +483,16 @@ let main ()       * thing a copy does is to remove the
template tag (since it's always
      * copied out of the cache directory).
      *)
-    tr `Copy 50 ((`Filename, output_filename) :: remove `Template itags);
-    tr `Copy 50 ((`Filename, tempfile) :: remove `Template itags);
+    tr `Copy ((`Filename, output_filename) :: remove `Template itags);
+    tr `Copy ((`Filename, tempfile) :: remove `Template itags);
 
     (* We can rename a file instead of copying, but don't rename the
-     * cache copy!  (XXX Also this is not free if copying across
-     * filesystems)
+     * cache copy!
      *)
     if is_not `Template then (
       if not output_is_block_dev then
-        tr `Rename 0 ((`Filename, output_filename) :: itags);
-      tr `Rename 0 ((`Filename, tempfile) :: itags);
+        tr `Move ((`Filename, output_filename) :: itags);
+      tr `Move ((`Filename, tempfile) :: itags)
     );
 
     if is `XZ then (
@@ -449,9 +500,9 @@ let main ()         * to the output file or to a temp file.
        *)
       if not output_is_block_dev then
-        tr `Pxzcat 80
+        tr `Pxzcat
           ((`Filename, output_filename) :: remove `XZ (remove `Template
itags));
-      tr `Pxzcat 80
+      tr `Pxzcat
         ((`Filename, tempfile) :: remove `XZ (remove `Template itags));
     )
     else (
@@ -462,11 +513,11 @@ let main ()        let old_size = Int64.of_string
(List.assoc `Size itags) in
       let headroom = 256L *^ 1024L *^ 1024L in
       if output_size >= old_size +^ headroom then (
-        tr `Virt_resize 100
+        tr `Virt_resize
           ((`Size, Int64.to_string output_size) ::
               (`Filename, output_filename) ::
               (`Format, output_format) :: (remove `Template itags));
-        tr `Virt_resize 100
+        tr `Virt_resize
           ((`Size, Int64.to_string output_size) ::
               (`Filename, tempfile) ::
               (`Format, output_format) :: (remove `Template itags))
@@ -485,15 +536,15 @@ let main ()         *)
       else if output_size > old_size && is_not `Template
               && List.mem_assoc `Format itags then
-        tr `Disk_resize 60 ((`Size, Int64.to_string output_size) :: itags);
+        tr `Disk_resize ((`Size, Int64.to_string output_size) :: itags);
 
       (* qemu-img convert is always possible, and quicker.  It doesn't
        * resize, but it does change the format.
        *)
-      tr `Convert 60
+      tr `Convert
         ((`Filename, output_filename) :: (`Format, output_format) ::
             (remove `Template itags));
-      tr `Convert 60
+      tr `Convert
         ((`Filename, tempfile) :: (`Format, output_format) ::
             (remove `Template itags));
     );
@@ -528,7 +579,7 @@ let main ()      in
     let print_task = function
       | `Copy -> printf "cp"
-      | `Rename -> printf "mv"
+      | `Move -> printf "mv"
       | `Pxzcat -> printf "pxzcat"
       | `Virt_resize -> printf "virt-resize"
       | `Disk_resize -> printf "qemu-img resize"
@@ -570,7 +621,7 @@ let main ()        let cmd = [ "cp"; ifile; ofile
] in
       if run_command cmd <> 0 then exit 1
 
-    | itags, `Rename, otags ->
+    | itags, `Move, otags ->
       let ifile = List.assoc `Filename itags in
       let ofile = List.assoc `Filename otags in
       let cmd = [ "mv"; ifile; ofile ] in
diff --git a/common/mlutils/unix_utils.ml b/common/mlutils/unix_utils.ml
index 3eb3ac890..ef51a7aa2 100644
--- a/common/mlutils/unix_utils.ml
+++ b/common/mlutils/unix_utils.ml
@@ -80,5 +80,9 @@ module StatVFS = struct
   external statvfs : string -> statvfs     
"guestfs_int_mllib_statvfs_statvfs"
 
+  let cifs_magic_number = 0xff534d42_L
+  let nfs_super_magic = 0x6969_L
+  let smb_super_magic = 0x517b_L
+
   let free_space { f_bsize = bsize; f_bavail = bavail } = bsize *^ bavail
 end
diff --git a/common/mlutils/unix_utils.mli b/common/mlutils/unix_utils.mli
index 385791cac..50affb4ae 100644
--- a/common/mlutils/unix_utils.mli
+++ b/common/mlutils/unix_utils.mli
@@ -116,6 +116,12 @@ module StatVFS : sig
       In case of non-Linux or non-POSIX, this call is emulated as best
       we can with missing fields returned as [-1]. *)
 
+  val cifs_magic_number : int64
+  val nfs_super_magic : int64
+  val smb_super_magic : int64
+  (** Magic numbers from some filesystems that might be found in the
+      [f_type] field.  See also [statfs(2)]. *)
+
   val free_space : statvfs -> int64
   (** [free_space (statvfs path)] returns the free space available on the
       filesystem that contains [path], in bytes. *)
-- 
2.13.2

On Monday, 2 October 2017 23:43:53 CEST Richard W.M. Jones
wrote:
> It started out as "this'll be just a simple fix ..."
> and turned into something a bit over-engineered in the end.
> 
> Here it is anyway.
(replying here, since my comments span both patches)

Even if we are supporting Linux only at the moment, IMHO it would be a
better idea to not expose f_type as part of the statvfs struct, since
it is not POSIX.  After all, it is not yet used by callers of StatVFS.

Instead, I'd implement directly is_remote in C (directly in builder),
calling directly fstatfs, and using the various *_MAGIC defines
provided by linux/magic.h.  This approach avoids duplicating in OCaml
the IDs of the various filesystems, and makes it easier to port to
other OSes.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL:
<http://listman.redhat.com/archives/libguestfs/attachments/20171003/9c756893/attachment.sig>