On 02/17/22 17:56, Eric Blake wrote: > On Thu, Feb 17, 2022 at 04:41:31PM +0000, Richard W.M. Jones wrote: >> On Thu, Feb 17, 2022 at 10:30:13AM -0600, Eric Blake wrote: >>> On Thu, Feb 17, 2022 at 02:36:43PM +0000, Richard W.M. Jones wrote: >>>> This filter doesn't call the next_open function in the non-TLS case, >>>> and therefore it never opens the plugin. This leaves the internal >>>> state of nbdkit a bit strange. There is no plugin context allocated, >>>> and the last filter in the chain has a context c_next pointer of NULL. >>>> >>>> This works, provided we intercept every possible callback, check the >>>> non-TLS case, and prevent it from calling the next function (because >>>> it would dereference the NULL c_next). >>>> >>>> To avoid a crash in backend_block_size we must therefore provide a >>>> .block_size callback in this filter. >>>> --- >>>> filters/tls-fallback/tls-fallback.c | 15 +++++++++++++++ >>>> 1 file changed, 15 insertions(+) >>> >>> ACK. >>> >>> Would you like to squash this in, and/or have me commit this separately? >> >> I was actually thinking about squashing my patches 1-4 together. >> They're all really the same change, but I kept them separate for ease >> of review. What do you think?
I think patch#2 should be squashed into patch#1 (and the commit messages should be merged). However, I'd suggest keeping each of patches #3 and #4 separate. Patch#2 fixes a bug exposed by patch#1, so patch#1 in itself doesn't just start introducing a feature, but also introduces (effectively) a bug (a crash), temporarily. That should be avoided. However, erecting a feature in multiple stages (patches #3 and #4) is what all feature series should do. It's natural that a feature doesn't (fully) work until the end of the series. Also, "ease of review" is not necessarily a one-time benefit; if we need to look at these patches in some years, I think we'll appreciate a fine granularity in the git history. Thanks, Laszlo > > Seems reasonable (I'll confirm it again when I get through reviewing 4). > >> >> But I think this patch: >> >>> commit 8c00ca2fe418aeecf0818feed227a72e76d87f18 >>> Author: Eric Blake <[email protected]> >>> Date: Thu Feb 17 10:24:50 2022 -0600 >>> >>> tls-fallback: Enhance comments about required callbacks > >> ... would stay separate, and you can push it before or after. > > Before - it is now commit 8c00ca2f ;) > _______________________________________________ Libguestfs mailing list [email protected] https://listman.redhat.com/mailman/listinfo/libguestfs
