On 02/04/22 17:44, Eric Blake wrote: > While writing tests for an nbdcopy bug, I found myself wanting a way > to easily view an entire image as data, but without disabling extents > support altogether. The existing extentlist filter can do this, but > requires a secondary file. > > Still an RFC because I need testsuite coverage similar to > test-nozero.sh (eek - we don't have any direct test of the noextent > filter, but only indirect coverage through other tests). Also, are > there any other extentmode=MODE values that might make sense? > --- > filters/noextents/nbdkit-noextents-filter.pod | 32 ++++++++++-- > filters/noextents/noextents.c | 50 ++++++++++++++++++- > 2 files changed, 75 insertions(+), 7 deletions(-) > > diff --git a/filters/noextents/nbdkit-noextents-filter.pod > b/filters/noextents/nbdkit-noextents-filter.pod > index 891b197d..aac2f097 100644 > --- a/filters/noextents/nbdkit-noextents-filter.pod > +++ b/filters/noextents/nbdkit-noextents-filter.pod > @@ -4,7 +4,7 @@ nbdkit-noextents-filter - disable extents in the underlying > plugin > > =head1 SYNOPSIS > > - nbdkit --filter=noextents plugin > + nbdkit --filter=noextents plugin [plugin-args...] [extentmode=MODE] > > =head1 DESCRIPTION > > @@ -23,9 +23,31 @@ performance (C<tmpfs> is known to be one such system). > > =head1 PARAMETERS > > -There are no parameters specific to nbdkit-noextents-filter. Any > -parameters are passed through to and processed by the underlying > -plugin in the normal way. > +The parameter C<extentmode> is optional, and controls which mode the > +filter will use. > + > +=over 4 > + > +=item B<extentmode=mask> > + > +Extent support is not advertised to the client; clients should not > +query for extent information, and must assume the entire disk is > +allocated. > + > +This is the default if the C<extentmode> parameter is not specified. > + > +=item B<extentmode=data> > + > +(nbdkit E<ge> 1.30) > + > +Extent support is advertised, but extent requests from the client will > +be answered with a claim that the entire disk forms a single allocated > +data extent. > + > +=back > + > +All other parameters are passed through to and processed by the > +underlying plugin in the normal way.
Is this necessary to spell out? (Looked at a random other filter's documentation, and didn't see such a statement, so I think it's the default.) (The same question applies to "plugin-args" in the synopsys, more or less...) > > =head1 FILES > > @@ -61,4 +83,4 @@ Richard W.M. Jones > > =head1 COPYRIGHT > > -Copyright (C) 2019 Red Hat Inc. > +Copyright (C) 2019-2022 Red Hat Inc. > diff --git a/filters/noextents/noextents.c b/filters/noextents/noextents.c > index f3044809..36231a35 100644 > --- a/filters/noextents/noextents.c > +++ b/filters/noextents/noextents.c > @@ -1,5 +1,5 @@ > /* nbdkit > - * Copyright (C) 2019 Red Hat Inc. > + * Copyright (C) 2019-2022 Red Hat Inc. > * > * Redistribution and use in source and binary forms, with or without > * modification, are permitted provided that the following conditions are > @@ -32,19 +32,65 @@ > > #include <config.h> > > +#include <string.h> > +#include <assert.h> > + > #include <nbdkit-filter.h> > > +static enum ExtentMode { > + MASK, > + DATA, > +} extentmode; > + > +static int > +noextents_config (nbdkit_next_config *next, nbdkit_backend *nxdata, > + const char *key, const char *value) > +{ > + if (strcmp (key, "extentmode") == 0) { > + if (strcmp (value, "mask") == 0) > + extentmode = MASK; > + else if (strcmp (value, "data") == 0) > + extentmode = DATA; > + else { > + nbdkit_error ("unknown extentmode '%s'", value); > + return -1; > + } > + return 0; > + } > + > + return next (nxdata, key, value); > +} > + > +#define noextents_config_help \ > + "extentmode=<MODE> One of 'mask' (default), 'data'.\n" > + > +/* Advertise desired extents support. */ > static int > noextents_can_extents (nbdkit_next *next, > void *handle) > { > - return 0; > + return extentmode == DATA; > +} > + > +/* Produce single data extent. */ > +static int > +noextents_extents (nbdkit_next *next, > + void *handle, uint32_t count, uint64_t offset, > + uint32_t flags, > + struct nbdkit_extents *ret_extents, > + int *err) > +{ > + assert (extentmode == DATA); > + return nbdkit_add_extent (ret_extents, offset, count, 0); > } I don't have the context in which this function may be called, in my head, but this implementation seems to reflect precisely the requested offset range back at the client, so a question arises: - What if the client requests an offset range that (at least partially) exceeds the size of the file? In that case, I think we should not report that range as existent. For example, the chunked block status query in virt-v2v asks for 2GB offset ranges, and it's expected that the returned extent list will not exceed the file size. (I understand that a server is permitted to cover a larger offset range than requested in its reply, unless LIBNBD_CMD_FLAG_REQ_ONE is set; however, this is different. Without LIBNBD_CMD_FLAG_REQ_ONE, the response may well be permitted to exceed the requested range, but it's still not permitted, AIUI, to report nonexistent data.) In short, I think we should call get_size() (?) on the underlying plugin (?), and truncate the requested range accordingly. > > static struct nbdkit_filter filter = { > .name = "noextents", > .longname = "nbdkit noextents filter", > + .config = noextents_config, > + .config_help = noextents_config_help, > .can_extents = noextents_can_extents, > + .extents = noextents_extents, > }; > > NBDKIT_REGISTER_FILTER(filter) > Thanks, Laszlo _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs