Libguestfs - May 2020 - Re: [PATCH nbdkit] DDRESCUE: MISC FIXES

A few fixes and a possible enhancement to the ddrescue filter.  If you
think these are all OK, I will squash it into your patch and push it.

Rich.

Use vector type to store map ranges.

Test filenames unique.

Remove some unused variables.

Break up long lines.
---
 filters/ddrescue/nbdkit-ddrescue-filter.pod |  6 ++-
 filters/ddrescue/ddrescue.c                 | 42 ++++++++++-----------
 tests/test-ddrescue-filter.sh               | 10 ++---
 3 files changed, 29 insertions(+), 29 deletions(-)

diff --git a/filters/ddrescue/nbdkit-ddrescue-filter.pod
b/filters/ddrescue/nbdkit-ddrescue-filter.pod
index 8210866b..601d58fa 100644
--- a/filters/ddrescue/nbdkit-ddrescue-filter.pod
+++ b/filters/ddrescue/nbdkit-ddrescue-filter.pod
@@ -4,9 +4,11 @@ nbdkit-ddrescue-filter - nbdkit filter for serving from
ddrescue dump
 
 =head1 SYNOPSIS
 
- nbdkit --filter=ddrescue plugin [plugin-args...] ddrescue-mapfile=file.map
+ nbdkit --filter=ddrescue plugin [plugin-args...]
+                                 ddrescue-mapfile=file.map
 
- nbdkit --filter=ddrescue file file=file.img ddrescue-mapfile=file.map
[plugin-args...]
+ nbdkit --filter=ddrescue file file=file.img ddrescue-mapfile=file.map
+                               [plugin-args...]
 
 =head1 DESCRIPTION
 
diff --git a/filters/ddrescue/ddrescue.c b/filters/ddrescue/ddrescue.c
index e4b51de4..a60f07e6 100644
--- a/filters/ddrescue/ddrescue.c
+++ b/filters/ddrescue/ddrescue.c
@@ -43,6 +43,7 @@
 #include <nbdkit-filter.h>
 
 #include "cleanup.h"
+#include "vector.h"
 
 struct range {
   int64_t start;
@@ -50,13 +51,14 @@ struct range {
   int64_t size;
   char status;
 };
+DEFINE_VECTOR_TYPE(ranges, struct range);
 
 struct mapfile {
   int ranges_count;
-  struct range *ranges;
+  ranges ranges;
 };
 
-static struct mapfile map = { 0, NULL };
+static struct mapfile map = { 0, empty_vector };
 
 static int
 parse_mapfile (const char *filename)
@@ -75,8 +77,6 @@ parse_mapfile (const char *filename)
   }
 
   while ((len = getline (&line, &linelen, fp)) != -1) {
-    const char *delim = " \t";
-    char *sp, *p;
     int64_t offset, length;
     char status;
 
@@ -95,7 +95,8 @@ parse_mapfile (const char *filename)
       continue;
     }
 
-    if (sscanf (line, "%" SCNi64 "\t%" SCNi64
"\t%c", &offset, &length, &status) == 3) {
+    if (sscanf (line, "%" SCNi64 "\t%" SCNi64
"\t%c",
+                &offset, &length, &status) == 3) {
       if (offset < 0) {
         nbdkit_error ("block offset must not be negative");
         return -1;
@@ -105,19 +106,17 @@ parse_mapfile (const char *filename)
         return -1;
       }
       if (status == '+') {
-        int i = map.ranges_count++;
-        map.ranges = realloc(map.ranges, map.ranges_count * sizeof(struct
range));
-        if (map.ranges == NULL) {
+        struct range new_range = { .start = offset, .end = offset + length - 1,
+                                   .size = length, .status = status };
+
+        if (ranges_append (&map.ranges, new_range) == -1) {
           nbdkit_error ("%s: ddrescue: realloc: %m", filename);
           goto out;
         }
-        map.ranges[i].start = offset;
-        map.ranges[i].end = offset + length - 1;
-        map.ranges[i].size = length;
-        map.ranges[i].status = status;
       }
 
-      nbdkit_debug ("%s: range: 0x%" PRIx64 " 0x%" PRIx64
" '%c'", filename, offset, length, status);
+      nbdkit_debug ("%s: range: 0x%" PRIx64 " 0x%" PRIx64
" '%c'",
+                    filename, offset, length, status);
     }
   }
 
@@ -133,9 +132,7 @@ parse_mapfile (const char *filename)
 static void
 ddrescue_unload (void)
 {
-  free (map.ranges);
-  map.ranges = NULL;
-  map.ranges_count = 0;
+  free (map.ranges.ptr);
 }
 
 static int
@@ -180,20 +177,21 @@ ddrescue_pread (struct nbdkit_next_ops *next_ops, void
*nxdata,
                 void *handle, void *buf, uint32_t count, uint64_t offset,
                 uint32_t flags, int *err)
 {
-  int i;
+  size_t i;
 
-  for (i = 0; i < map.ranges_count; i++) {
-    if (map.ranges[i].status != '+')
+  for (i = 0; i < map.ranges.size; i++) {
+    if (map.ranges.ptr[i].status != '+')
       continue;
-    if (offset >= map.ranges[i].start && offset <=
map.ranges[i].end) {
-      if (offset + count - 1 <= map.ranges[i].end) {
+    if (offset >= map.ranges.ptr[i].start && offset <=
map.ranges.ptr[i].end) {
+      if (offset + count - 1 <= map.ranges.ptr[i].end) {
         /* entirely contained within this range */
         return next_ops->pread (nxdata, buf, count, offset, flags, err);
       }
     }
   }
   /* read was not fully covered */
-  nbdkit_debug ("ddrescue: pread: range: 0x%" PRIx64 " 0x%"
PRIx32 " failing with EIO", offset, count);
+  nbdkit_debug ("ddrescue: pread: range: 0x%" PRIx64 " 0x%"
PRIx32
+                " failing with EIO", offset, count);
   *err = EIO;
   return -1;
 }
diff --git a/tests/test-ddrescue-filter.sh b/tests/test-ddrescue-filter.sh
index f08bbd4e..10a78e4d 100755
--- a/tests/test-ddrescue-filter.sh
+++ b/tests/test-ddrescue-filter.sh
@@ -40,13 +40,13 @@ set -x
 requires nbdsh --version
 
 sock=`mktemp -u`
-files="data-file.pid $sock data-ddrescue.txt ddrescue-test1.map"
+files="ddrescue.pid $sock ddrescue.txt ddrescue-test1.map"
 rm -f $files
 cleanup_fn rm -f $files
 
-rm -f data-ddrescue.txt
+rm -f ddrescue.txt
 for i in {0..1000}; do
-    printf "ddrescue" >> data-ddrescue.txt
+    printf "ddrescue" >> ddrescue.txt
 done
 
 echo "#
@@ -59,12 +59,12 @@ echo "#
 
 
 # Run nbdkit.
-start_nbdkit -P data-file.pid -U $sock \
+start_nbdkit -P ddrescue.pid -U $sock \
        --filter=ddrescue data \
        ddrescue-mapfile="ddrescue-test1.map"\
        size=1M \
        data="
-   @0x000 <data-ddrescue.txt
+   @0x000 <ddrescue.txt
    "
 
 nbdsh --connect "nbd+unix://?socket=$sock" \
-- 
2.25.0

On 5/22/20 2:53 PM, Richard W.M. Jones wrote:
> Use vector type to store map ranges.
> 
> Test filenames unique.
> 
> Remove some unused variables.
> 
> Break up long lines.
> ---
> @@ -95,7 +95,8 @@ parse_mapfile (const char *filename)
>         continue;
>       }
>   
> -    if (sscanf (line, "%" SCNi64 "\t%" SCNi64
"\t%c", &offset, &length, &status) == 3) {
> +    if (sscanf (line, "%" SCNi64 "\t%" SCNi64
"\t%c",
> +                &offset, &length, &status) == 3) {
sscanf() cannot detect integer overflow.  If you care about detecting 
overflow, you'll have to rewrite this into an open-coded loop using 
things like nbdkit_parse_size().

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

This is upstream now.  Thanks for your contribution to nbdkit.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html