Libguestfs - Apr 2017 - Re: [PATCH v2 0/2] daemon: Move the useful 'is_zero' function into common code.

v1 -> v2:

The first patch is the same (the pure refactoring), but in the second
patch I implement Eric Blake's suggested version.

Rich.

This is largely a simple refactoring, but it combines another
definition of this function from virt-builder which had a slightly
different prototype.
---
 builder/pxzcat-c.c         | 20 +-------------------
 daemon/daemon.h            | 18 ------------------
 lib/guestfs-internal-all.h | 18 ++++++++++++++++++
 3 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/builder/pxzcat-c.c b/builder/pxzcat-c.c
index 33923b532..ef37849ed 100644
--- a/builder/pxzcat-c.c
+++ b/builder/pxzcat-c.c
@@ -409,24 +409,6 @@ parse_indexes (value filenamev, int fd)
   return combined_index;
 }
 
-/* Return true iff the buffer is all zero bytes.
- *
- * Note that gcc is smart enough to optimize this properly:
- *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
- */
-static inline int
-is_zero (const unsigned char *buffer, size_t size)
-{
-  size_t i;
-
-  for (i = 0; i < size; ++i) {
-    if (buffer[i] != 0)
-      return 0;
-  }
-
-  return 1;
-}
-
 struct global_state {
   /* Current iterator.  Threads update this, but it is protected by a
    * mutex, and each thread takes a copy of it when working on it.
@@ -681,7 +663,7 @@ worker_thread (void *vp)
         /* Don't write if the block is all zero, to preserve output file
          * sparseness.  However we have to update oposition.
          */
-        if (!is_zero (outbuf, wsz)) {
+        if (!is_zero ((char *) outbuf, wsz)) {
           if (xpwrite (global->ofd, outbuf, wsz, oposition) == -1) {
             perror (global->outputfile);
             return &state->status;
diff --git a/daemon/daemon.h b/daemon/daemon.h
index 332e2966a..a317139a5 100644
--- a/daemon/daemon.h
+++ b/daemon/daemon.h
@@ -327,24 +327,6 @@ extern void pulse_mode_cancel (void);
  */
 extern void notify_progress_no_ratelimit (uint64_t position, uint64_t total,
const struct timeval *now);
 
-/* Return true iff the buffer is all zero bytes.
- *
- * Note that gcc is smart enough to optimize this properly:
- *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
- */
-static inline int
-is_zero (const char *buffer, size_t size)
-{
-  size_t i;
-
-  for (i = 0; i < size; ++i) {
-    if (buffer[i] != 0)
-      return 0;
-  }
-
-  return 1;
-}
-
 /* Helper for functions that need a root filesystem mounted.
  * NB. Cannot be used for FileIn functions.
  */
diff --git a/lib/guestfs-internal-all.h b/lib/guestfs-internal-all.h
index 0c841ff67..4f7433332 100644
--- a/lib/guestfs-internal-all.h
+++ b/lib/guestfs-internal-all.h
@@ -89,6 +89,24 @@
 #define xdr_uint32_t xdr_u_int32_t
 #endif
 
+/* Return true iff the buffer is all zero bytes.
+ *
+ * Note that gcc is smart enough to optimize this properly:
+ *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
+ */
+static inline int
+is_zero (const char *buffer, size_t size)
+{
+  size_t i;
+
+  for (i = 0; i < size; ++i) {
+    if (buffer[i] != 0)
+      return 0;
+  }
+
+  return 1;
+}
+
 /* Macro which compiles the regexp once when the program/library is
  * loaded, and frees it when the library is unloaded.
  */
-- 
2.12.0

As suggested by Eric Blake.
---
 lib/guestfs-internal-all.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/guestfs-internal-all.h b/lib/guestfs-internal-all.h
index 4f7433332..7da93828e 100644
--- a/lib/guestfs-internal-all.h
+++ b/lib/guestfs-internal-all.h
@@ -32,6 +32,8 @@
 #ifndef GUESTFS_INTERNAL_ALL_H_
 #define GUESTFS_INTERNAL_ALL_H_
 
+#include <string.h>
+
 /* This is also defined in <guestfs.h>, so don't redefine it. */
 #if defined(__GNUC__) && !defined(GUESTFS_GCC_VERSION)
 # define GUESTFS_GCC_VERSION \
@@ -89,20 +91,18 @@
 #define xdr_uint32_t xdr_u_int32_t
 #endif
 
-/* Return true iff the buffer is all zero bytes.
- *
- * Note that gcc is smart enough to optimize this properly:
- *
http://stackoverflow.com/questions/1493936/faster-means-of-checking-for-an-empty-buffer-in-c/1493989#1493989
- */
+/* Return true iff the buffer is all zero bytes. */
 static inline int
 is_zero (const char *buffer, size_t size)
 {
   size_t i;
+  const size_t limit = MIN (size, 16);
 
-  for (i = 0; i < size; ++i) {
-    if (buffer[i] != 0)
+  for (i = 0; i < limit; ++i)
+    if (buffer[i])
       return 0;
-  }
+  if (size != limit)
+    return !memcmp (buffer, buffer + 16, size - 16);
 
   return 1;
 }
-- 
2.12.0

On Wednesday, 19 April 2017 18:46:37 CEST Richard W.M. Jones
wrote:
> v1 -> v2:
> 
> The first patch is the same (the pure refactoring), but in the second
> patch I implement Eric Blake's suggested version.
LGTM -- can you please add Eric's text to the comment of is_zero, so
it is easier to remember why the 16-bytes trick is done?

Thanks,
-- 
Pino Toscano

On Thu, Apr 20, 2017 at 02:30:52PM +0200, Pino Toscano
wrote:
> On Wednesday, 19 April 2017 18:46:37 CEST Richard W.M. Jones wrote:
> > v1 -> v2:
> > 
> > The first patch is the same (the pure refactoring), but in the second
> > patch I implement Eric Blake's suggested version.
> 
> LGTM -- can you please add Eric's text to the comment of is_zero, so
> it is easier to remember why the 16-bytes trick is done?
Done, thanks.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org