On Fri, Jun 10, 2022 at 10:03:23PM +0100, Richard W.M. Jones wrote: > On Fri, Jun 10, 2022 at 03:21:17PM -0500, Eric Blake wrote: > > On Fri, Jun 10, 2022 at 05:21:19PM +0100, Richard W.M. Jones wrote: > > > > > > Series looks good: > > > > > > Acked-by: Richard W.M. Jones <[email protected]> > > > > > > I think nbdkit-ext2-filter is still wrong (although at least it should > > > no longer corrupt disk images) because it unnecessarily calls > > > next->can_zero, so it might be worth dropping those two lines. > > > > ext2 does not have an interface (yet?) for emulating fallocate() > > operations (or other bulk-zeroing operation) against a file; if it > > _did_ have one, then that's what ext2's .zero should do. But we _do_ > > want to allow the compressed transmission effects of > > NBD_CMD_WRITE_ZEROES over the network, so having .can_zero return > > NBDKIT_ZERO_EMULATE is right. On the other hand, ext2 DOES have a way > > for the file system itself to request bulk-zeroing of a portion of the > > underlying disk, so we _do_ call into the plugin's next->zero(), which > > means we _do_ need to check next->can_zero() up front (see the io.c > > callback io_zeroout). I don't see any bugs in this area, once this > > series is in. > > I mean these two lines: > > https://gitlab.com/nbdkit/nbdkit/-/blob/b4b8bd78ee66e5e1bc3d9b5464b10be0719445f1/filters/ext2/ext2.c#L342-L343 > > Before your patch series, they avoid the same assertion fail that we > saw in the luks filter. The mechanism is that when ZERO_EMULATE > caused filter zero to get forwarded to the plugin, because plugin > can_zero had never been called, the assertion fired. Those two lines > initialize plugin can_zero, removing the assertion but causing the > .zero call to be forwarded to the plugin unmodified (randomly zeroing > some part of the disk). > > After your patch series, the two lines just do nothing at all.
No, they are still useful. If we don't call them during handshake, then the later call to next->can_zero in io.c may fail without setting errno. Any time we want to guarantee a next->can_* function will succeed at all later points where we need a sane errno, we do so by pre-caching it during handshake. > > Anyway, I will remove them. Please don't. But adding comments for why they are important is okay. I also fixed another bug today: the eval plugin is smart enough to synthesize several .can_* helpers if the corresponding function exists (for example, with sh, you have to explicitly implement can_flush to get flush called, but not with eval); but the eval plugin wasn't doing a good synthesis for .can_cache. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
