[Libguestfs] [PATCH nbdkit v2 2/7] tls-fallback: Fix filter for new .block_size callback

[Laszlo Ersek]

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 <eblake at redhat.com>
>>> 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 ;)
>