Skip to content

Conversation

@mike-scott
Copy link
Contributor

@mike-scott mike-scott commented Oct 16, 2017

With the introduction of CoAP and other protocols, URL parsing is
needed when HTTP_PARSER is not. Let's split out the existing
functionality of URL parsing into it's own CONFIG and let
HTTP_PARSER use it by automatically selecting HTTP_PARSER_URL when
HTTP_PARSER is enabled.

Signed-off-by: Michael Scott [email protected]

With the introduction of CoAP and other protocols, URL parsing is
be needed when HTTP_PARSER is not.  Let's split out the existing
functionality of URL parsing into it's own CONFIG and let
HTTP_PARSER use it by automatically selecting HTTP_PARSER_URL when
HTTP_PARSER is enabled.

Signed-off-by: Michael Scott <[email protected]>
@mike-scott
Copy link
Contributor Author

mike-scott commented Oct 16, 2017

This commit is will be the ancestor of #4208 which will drop the first commit and be slightly reworked once this is merged.

@mike-scott
Copy link
Contributor Author

@nashif In #4208 you requested a different approach to splitting out the URL parsing code. Please let me know if this is also the wrong approach and we can discuss.

One concern to note: Future pulls from the upstream repo (https://github.com/nodejs/http-parser/releases/tag/v2.7.1) will be harder to apply this way. Where the #ifdef approach might be easier.

@mike-scott mike-scott requested a review from rsalveti October 16, 2017 23:56
Copy link
Contributor

@rbtchc rbtchc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jukkar
Copy link
Member

jukkar commented Oct 17, 2017

One concern to note: Future pulls from the upstream repo (https://github.com/nodejs/http-parser/releases/tag/v2.7.1) will be harder to apply this way. Where the #ifdef approach might be easier.

I agree, it will be much harder to take any upstream http parser code after this. But the code is in subsys/net/lib/http instead of ext/lib so it looks like this merging was never really expected anyway.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nashif nashif merged commit 1d36225 into zephyrproject-rtos:master Oct 17, 2017
@mike-scott mike-scott deleted the http-url-parser-split branch October 17, 2017 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants