Libguestfs - Mar 2021 - [PATCH libnbd 0/4] Various nbdcopy cleanups

Few minor refactorings, hopefully makeing the code more clear and less error
prone.

Nir Soffer (4):
  copy: nbd-ops.c: Less convoluted error handling
  copy: file-ops.c: Remove unneeded check
  copy: file-ops.c: Remove unneeded arguments
  copy: file-ops.c: Rename clean up label

 copy/file-ops.c | 41 +++++++++++++++++++++--------------------
 copy/nbd-ops.c  | 27 ++++++++++++++++-----------
 2 files changed, 37 insertions(+), 31 deletions(-)

-- 
2.26.2

Jumping back to error label works, but is harder to follow. Move the
error handlers to the end of the function.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/nbd-ops.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
index 2af09b2..cb11e64 100644
--- a/copy/nbd-ops.c
+++ b/copy/nbd-ops.c
@@ -136,18 +136,19 @@ nbd_rw_create_subprocess (const char **argv, size_t argc,
direction d)
 
   /* We have to copy the args so we can null-terminate them. */
   for (i = 0; i < argc; ++i) {
-    if (const_string_vector_append (&rwn->argv, argv[i]) == -1) {
-    memory_error:
-      perror ("realloc");
-      exit (EXIT_FAILURE);
-    }
+    if (const_string_vector_append (&rwn->argv, argv[i]) == -1)
+      goto error;
   }
   if (const_string_vector_append (&rwn->argv, NULL) == -1)
-    goto memory_error;
+    goto error;
 
   open_one_nbd_handle (rwn);
 
   return &rwn->rw;
+
+error:
+  perror ("realloc");
+  exit (EXIT_FAILURE);
 }
 
 static void
@@ -384,14 +385,18 @@ nbd_ops_get_polling_fd (struct rw *rw, uintptr_t index,
   nbd = rwn->handles.ptr[index];
 
   *fd = nbd_aio_get_fd (nbd);
-  if (*fd == -1) {
-  error:
-    fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
-    exit (EXIT_FAILURE);
-  }
+  if (*fd == -1)
+    goto error;
+
   *direction = nbd_aio_get_direction (nbd);
   if (*direction == -1)
     goto error;
+
+  return;
+
+error:
+  fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
+  exit (EXIT_FAILURE);
 }
 
 static void
-- 
2.26.2

This function is called only from page_cache_evict(), which already
check that we could map the cached pages. Add an assert to document this
assumption.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/file-ops.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/copy/file-ops.c b/copy/file-ops.c
index 0995e92..57999cb 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -94,7 +94,7 @@ page_size_init (void)
 /* Load the page cache map for a particular file into
  * rwf->cached_pages.  Only used when reading files.  This doesn't
  * fail: if a system call fails then rwf->cached_pages.size will be
- * zero which is handled in page_was_cached.
+ * zero which is handled in page_cache_evict.
  */
 static inline void
 page_cache_map (struct rw_file *rwf, int fd, int64_t size)
@@ -118,19 +118,16 @@ page_cache_map (struct rw_file *rwf, int fd, int64_t size)
   munmap (ptr, size);
 }
 
-/* Test if a single page of the file was cached before nbdcopy ran. */
+/* Test if a single page of the file was cached before nbdcopy ran.
+ * Valid only if we mapped the cached pages.
+ */
 static inline bool
 page_was_cached (struct rw_file *rwf, uint64_t offset)
 {
   uint64_t page = offset / page_size;
-  if (page < rwf->cached_pages.size)
-    return (rwf->cached_pages.ptr[page] & 1) != 0;
-  else
-    /* This path is taken if we didn't manage to map the input file
-     * for any reason.  In this case assume that pages were mapped so
-     * we will not evict them: essentially fall back to doing nothing.
-     */
-    return true;
+  assert (page < rwf->cached_pages.size);
+
+  return (rwf->cached_pages.ptr[page] & 1) != 0;
 }
 
 /* Evict file contents from the page cache if they were not present in
@@ -142,6 +139,10 @@ page_cache_evict (struct rw_file *rwf, uint64_t
orig_offset, size_t orig_len)
   uint64_t offset, n;
   size_t len;
 
+  /* If we didn't manage to map the input file for any reason, assume
+   * that pages were mapped so we will not evict them: essentially fall
+   * back to doing nothing.
+   */
   if (rwf->cached_pages.size == 0) return;
 
   /* Only bother with whole pages. */
-- 
2.26.2

page_cache_map() must operate on the file fd and size. Accepting these
as separate arguments mean we can call this function with the wrong fd
and size.

Using rwf->fd and rwf->rw.size is more verbose, but it makes clear whats
going on.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/file-ops.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/copy/file-ops.c b/copy/file-ops.c
index 57999cb..c6fad06 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -97,25 +97,25 @@ page_size_init (void)
  * zero which is handled in page_cache_evict.
  */
 static inline void
-page_cache_map (struct rw_file *rwf, int fd, int64_t size)
+page_cache_map (struct rw_file *rwf)
 {
   void *ptr;
 
-  if (size == 0) return;
+  if (rwf->rw.size == 0) return;
 
-  ptr = mmap (NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+  ptr = mmap (NULL, rwf->rw.size, PROT_READ, MAP_PRIVATE, rwf->fd, 0);
   if (ptr == (void *)-1) return;
 
-  const size_t veclen = ROUND_UP (size, page_size) / page_size;
+  const size_t veclen = ROUND_UP (rwf->rw.size, page_size) / page_size;
 
   if (byte_vector_reserve (&rwf->cached_pages, veclen) == -1)
     goto err;
-  if (mincore (ptr, size, rwf->cached_pages.ptr) == -1)
+  if (mincore (ptr, rwf->rw.size, rwf->cached_pages.ptr) == -1)
     goto err;
 
   rwf->cached_pages.size = veclen;
  err:
-  munmap (ptr, size);
+  munmap (ptr, rwf->rw.size);
 }
 
 /* Test if a single page of the file was cached before nbdcopy ran.
@@ -240,7 +240,7 @@ file_create (const char *name, int fd,
 
 #if PAGE_CACHE_MAPPING
   if (d == READING)
-    page_cache_map (rwf, fd, rwf->rw.size);
+    page_cache_map (rwf);
 #endif
 
   return &rwf->rw;
-- 
2.26.2

Using error: label for code used in normal flow is confusing. This kind
of style was the reason to the famous "goto fail" Apple bug. Lets call
it "out" to make the intent of the code more clear.

Signed-off-by: Nir Soffer <nsoffer at redhat.com>
---
 copy/file-ops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/copy/file-ops.c b/copy/file-ops.c
index c6fad06..7c42c7c 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -109,12 +109,12 @@ page_cache_map (struct rw_file *rwf)
   const size_t veclen = ROUND_UP (rwf->rw.size, page_size) / page_size;
 
   if (byte_vector_reserve (&rwf->cached_pages, veclen) == -1)
-    goto err;
+    goto out;
   if (mincore (ptr, rwf->rw.size, rwf->cached_pages.ptr) == -1)
-    goto err;
+    goto out;
 
   rwf->cached_pages.size = veclen;
- err:
+ out:
   munmap (ptr, rwf->rw.size);
 }
 
-- 
2.26.2