Hi Willy, > finally back to this! Overall it's a great and very clean series, I really > want to thank you for this high quality work! Thanks for the compliment, really glad to hear! :)
> Yeah it initially gave me a bit of head scratching when reading this part but > I understood what you did and find that it pretty much makes sense after all. > It's not necessarily a hack given that what you've done consists in passing a > pre-parsed argument internally. It's not much different from the few cases > where we emulate older functions or actions using new mechanisms, building > some config elements on the fly just to parse them. Okay, great if there is already similar code. Hack was a bit of an overstatement by me, at least if works a bit different than usually. > I'm fine with this, however I find that the doc is not very clear about what > is permitted I agree that doc needs some more details. I added the note about the iterations and described all the symbolic constants, see at the end of this mail. > Unless you're having any objection, I'm going to flip type and len below: Sorry, that was an oversight on my end. Sure, go ahead. I actually thought I would have arranged it that way, but it somehow slipped my view. > I tend to *prefer* to have them separately for long series that take a lot of > time to review and cannot be done at once. But this series could be reviewed > at once, most patches remain small enough so that's no big deal. OK, great. But I try to keep it in mind for future work anyway. > In this case it could be clarified in the doc that the sample fetch functions > for authority/uniqueid only return the first of each, and that > fc_pp_tlv() iterates over all occurrences of the requested ID. This would > then place a clear separation between trying to extract THE authority, or > checking ALL TLVs of type AUTHORITY. > What do you think ? Good idea, agreed. That also aligns with my goals to keep the existing behavior as much as possible. > If you're OK with this, I'd appreciate it if you could send a few informal > incremental patches that I'd squash into yours. I'm OK with this. :-) The first patch (0001) should be squashed into the commit that introduces the fetch fc_pp_tlv. I guess it fits there because it actually retains and documents prior behavior. The behavior regarding duplicates is also already present, therefore I already added corresponding docs there. The second one (0002) could then be applied to the last patch with the introduction of the constants. Would that work for you? Best, Alexander
0002-Extend-docs-of-fc_pp_tlv-with-constants.patch
Description: 0002-Extend-docs-of-fc_pp_tlv-with-constants.patch
0001-Restrict-fc_pp_authority-and-fc_pp_unique_id-to-firs.patch
Description: 0001-Restrict-fc_pp_authority-and-fc_pp_unique_id-to-firs.patch
