[Libguestfs] [PATCH] WIP: ddrescue mapfile filter

[Richard W.M. Jones]

On Fri, May 01, 2020 at 07:14:32PM +0200, François Revol wrote:
> This allows to overlay bad sectors according to the mapfile generated by
> ddrescue, to then see where sectors are used using fsck and trying to
> copy files around.
> 
> Signed-off-by: François Revol <revol at free.fr>
...
> +nbdkit_ddrescue_filter_la_LDFLAGS = \
> +	-module -avoid-version -shared \

We recently added $(SHARED_LDFLAGS) to the end of this line.
Compare filters/ip/Makefile.am to your file.

> diff --git a/filters/ddrescue/ddrescue.c b/filters/ddrescue/ddrescue.c
> new file mode 100644
> index 00000000..a0e49e3c
> --- /dev/null
> +++ b/filters/ddrescue/ddrescue.c
> @@ -0,0 +1,218 @@
> +/* nbdkit
> + * Copyright (C) 2018-2020 Red Hat Inc.

In several files and the man page you're assigning copyright over
to Red Hat, but it's probably better to copyright them to yourself
and/or your organisation.

> +struct range {
> +  int64_t start;
> +  int64_t end;
> +  int64_t size;
> +  char status;
> +};
> +
> +struct mapfile {
> +  int ranges_count;
> +  struct range *ranges;
> +};
>
> +static struct mapfile map = { 0, NULL };

There are actually two generic types in common/ which might help here.
There's a vector type for building lists:

  https://github.com/libguestfs/nbdkit/blob/master/common/utils/vector.h

Example usage:

  https://github.com/libguestfs/nbdkit/blob/master/plugins/split/split.c

And there's a regions type (built on top of vector) for building
contiguous ranges covering a disk, which has a fast look-up method:

  https://github.com/libguestfs/nbdkit/blob/master/common/regions/regions.h

Of course it's optional to use these but they will probably simplify
your code.

> +
> +
> +static void
> +ddrescue_load (void)
> +{
> +}

Double blank line before this function, but also this function is
empty so you don't need to include it (even though there is an .unload
function - .unload will still be called even if .load is missing).

> +/* We need this because otherwise the layer below can_write is called
> + * and that might return true (eg. if the plugin has a pwrite method
> + * at all), resulting in writes being passed through to the layer
> + * below.  This is possibly a bug in nbdkit.
> + */

I think this is working as intended, not a bug.  However it's good to
comment on why the can_write function is needed.

[...]

> +=back
> +
> +=head1 VERSION
> +
> +C<nbdkit-ddrescue-filter> first appeared in nbdkit 1.21.

It'll be 1.22 (next stable version after 1.20).

> +=head1 SEE ALSO
> +
> +L<nbdkit(1)>,
> +L<nbdkit-file-plugin(1)>,
> +L<nbdkit-filter(3)>,
> +L<ddrescue(1)>,
> +L<https://www.gnu.org/software/ddrescue/manual/ddrescue_manual.html>.
> +
> +=head1 AUTHORS
> +
> +François Revol
> +
> +Richard W.M. Jones

I guess the author is only you!

> +=head1 COPYRIGHT
> +
> +Copyright (C) 2020 Red Hat Inc.

See note about copyright above.

It generally looks fine to me.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top