Skip to content
This repository has been archived by the owner on Nov 6, 2022. It is now read-only.

Support ICE/1.x for SOURCE requests. #431

Merged
merged 1 commit into from
Oct 29, 2018
Merged

Conversation

bnoordhuis
Copy link
Member

Normal clients sends "GET / HTTP/1.0" but Shoutcast clients send
"SOURCE / ICE/1.0". Accept that as an alternative but only for
SOURCE requests.

Fixes: #410

@bnoordhuis
Copy link
Member Author

@indutny Can I get a LGTM? Congrats on the 5-0, by the way! #футбола

@bnoordhuis bnoordhuis requested a review from indutny October 29, 2018 12:45
Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for not seeing this before.

UPDATE_STATE(s_req_http_I);
break;
}
/* fall through */
Copy link
Contributor

Choose a reason for hiding this comment

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

this falls-through to break; but looks like maybe it was intended to fall-through to SET_ERRNO(HPE_INVALID_CONSTANT)

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, you're right. Maybe I botched a rebase because I don't recall writing it like that. I'll fix it up before landing, thanks for pointing it out.

Normal clients sends "GET / HTTP/1.0" but Shoutcast clients send
"SOURCE / ICE/1.0".  Accept that as an alternative but only for
SOURCE requests.

Fixes: nodejs#410
PR-URL: nodejs#431
Reviewed-By: Fedor Indutny <fedor@indutny.com>
Reviewed-By: Pierce Lopez <pierce.lopez@gmail.com>
@bnoordhuis bnoordhuis closed this Oct 29, 2018
@bnoordhuis bnoordhuis deleted the fix410 branch October 29, 2018 17:33
@bnoordhuis bnoordhuis merged commit 4dae120 into nodejs:master Oct 29, 2018
@bnoordhuis
Copy link
Member Author

Landed with an extra test in 4dae120.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants