Re: [PATCH] connect: also update offset for features without values

[Andrzej Hunt <andrzej@ahunt.org>]

On 18/09/2021 17:53, Taylor Blau wrote:
>> parse_feature_value() does not update offset if the feature being
>> searched for does not specify a value. A loop that uses
>> parse_feature_value() to find a feature which was specified without a
>> value therefore might never exit (such loops will typically use
>> next_server_feature_value() as opposed to parse_feature_value() itself).
>> This usually isn't an issue: there's no point in using
>> next_server_feature_value() to search for repeated instances of the same
>> capability unless that capability typically specifies a value - but a
>> broken server could send a response that omits the value for a feature
>> even when we are expecting a value.
> 
> It may be worth adding a little detail here. parse_feature_value takes
> an offset, and uses it to seek past the point in features_list that
> we've already seen. But if we get a value-less feature, then offset is
> never updated, and we'll keep parsing the same thing over and over in a
> loop.
> 
> (I know that you know all of that, but I think it is worth spelling out
> a little more clearly in the patch message).

Good point - I've tried to improve this for V2 (I've mostly just copied 
your description verbatim).

> 
>> Therefore we add an offset update calculation for the no-value case,
>> which helps ensure that loops using next_server_feature_value() will
>> always terminate.
> 
>> next_server_feature_value(), and the offset calculation, were first
>> added in 2.28 in:
>>    2c6a403d96 (connect: add function to parse multiple v1 capability values, 2020-05-25)
> 
> This line wrapping is a little odd, but not a big deal.

I'll fix this for V2 - I think I tried too hard to make this look nice, 
but putting the reference inline does look better (and I've now realised 
this seems to be the usual way of doing things here).

ATB,

Andrzej