git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Andrzej Hunt <andrzej@ahunt.org>
Date
Sep 26, 2021, 15:14 UTC
Message-ID
<14ff5661-e06a-8348-7088-387fa7e3e094@ahunt.org>
In-Reply-To
<YUYLXKN8U9AMa5ke@nand.local>
On 18/09/2021 17:53, Taylor Blau wrote:
Show 19 quoted lines
>> 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).

Show 10 quoted lines
> 
>> 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
Previous: Junio C HamanoNext: brian m. carlson
Message 9 of 17 in “connect: also update offset for features without values”
  1. connect: also update offset for features without valuesAndrzej Hunt via GitGitGadget, Sep 18, 2021
  2. Taylor BlauSep 18, 2021
  3. Jeff KingSep 18, 2021
  4. Taylor BlauSep 18, 2021
  5. Eric SunshineSep 19, 2021
  6. Jeff KingSep 19, 2021
  7. Eric SunshineSep 19, 2021
  8. Junio C HamanoSep 19, 2021
  9. Andrzej HuntSep 26, 2021
  10. brian m. carlsonSep 18, 2021
  11. Junio C HamanoSep 23, 2021
  12. Jeff KingSep 23, 2021
  13. Junio C HamanoSep 23, 2021
  14. Jeff KingSep 23, 2021
  15. Andrzej HuntSep 26, 2021
  16. connect: also update offset for features without valuesAndrzej Hunt via GitGitGadget, Sep 26, 2021
  17. Jeff KingSep 27, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.