GNOME Bugzilla – Bug 752866
souphttpsrc: allow empty http proxy string
Last modified: 2015-08-16 13:41:30 UTC
if the http proxy env is set as null string like below, it makes error. http_proxy="" 0:00:00.186924920 9713 0xa3ba4c0 WARN souphttpsrc gstsouphttpsrc.c:1535:gst_soup_http_src_parse_status:<source> error: Could not resolve server name. 0:00:00.186971809 9713 0xa3ba4c0 WARN souphttpsrc gstsouphttpsrc.c:1535:gst_soup_http_src_parse_status:<source> error: Cannot resolve hostname (content.bitsontherun.com) (2), URL: http://content.bitsontherun.com/videos/ntPYsD4L-1ahmry41.mp4, Redirect to: (NULL) 0:00:00.187047093 9713 0xa3ba4c0 WARN souphttpsrc gstsouphttpsrc.c:1535:gst_soup_http_src_parse_status:<source> error: Could not resolve server name. 0:00:00.187060401 9713 0xa3ba4c0 WARN souphttpsrc gstsouphttpsrc.c:1535:gst_soup_http_src_parse_status:<source> error: Cannot resolve hostname (content.bitsontherun.com) (2), URL: http://content.bitsontherun.com/videos/ntPYsD4L-1ahmry41.mp4, Redirect to: (NULL) ERROR Could not resolve server name. for http://content.bitsontherun.com/videos/ntPYsD4L-1ahmry41.mp4 ERROR debug information: gstsouphttpsrc.c(1535): gst_soup_http_src_parse_status (): /GstPlayBin:playbin/GstURIDecodeBin:uridecodebin0/GstSoupHTTPSrc:source: Cannot resolve hostname (content.bitsontherun.com) (2), URL: http://content.bitsontherun.com/videos/ntPYsD4L-1ahmry41.mp4, Redirect to: (NULL) 0:00:00.187127430 9713 0xa3ba4c0 WARN basesrc gstbasesrc.c:2957:gst_base_src_loop:<source> error: Internal data flow error. 0:00:00.187135160 9713 0xa3ba4c0 WARN basesrc gstbasesrc.c:2957:gst_base_src_loop:<source> error: streaming task paused, reason error (-5)
Created attachment 308145 [details] [review] souphttpsrc: add checking null string add checking null string before setting proxy
Review of attachment 308145 [details] [review]: Makes sense but perhaps you could elaborate on the commit message a bit? What is your use case for this?
Created attachment 308210 [details] [review] souphttpsrc: check null string before setting proxy I've got report about this error from tizen sdk team. I could reproduce the error after adding http_proxy with null string like below. export http_proxy="" In that case, the g_getenv return real address not null but it includes null string. I know this is not normal but it could be happened in any system or by any user. I've attached new patch with detail commit log. Thank you.
Created attachment 308211 [details] [review] souphttpsrc: check null string before setting proxy fix typo in commit log.
Review of attachment 308211 [details] [review]: Good catch but: ::: ext/soup/gstsouphttpsrc.c @@ +2046,3 @@ if (g_str_has_prefix (uri, "http://")) { src->proxy = soup_uri_new (uri); } else { I think it would be better to move the test here as else if (uri[0] != '\0') { ... } else return FALSE; Also, it might make sense to extend the test to avoid setting nonsensical values altogether but that might be too much, just a thought.
Created attachment 308431 [details] [review] souphttpsrc: check null string before setting proxy I've added new patch according to the guidance. Thank you.
The earlier version with the early return is actually closer to GStreamer style. My thoughts on this: a) We should just check for the empty string in gst_soup_http_src_init() where we read the env var: if (proxy && *proxy && !set_proxy (...)) b) When setting the proxy via the GObject property, both NULL and an empty string should be valid inputs and should lead to any existing proxy to be unset. This means that the == NULL check in _set_property() is not right, it should go into set_proxy() in any case, and set_proxy() should unset the old proxy and then bail out if the string is NULL or empty. c) We should either remove the return value from _set_proxy() entirely or (better, IMHO) return (src->proxy != NULL) to check that soup_uri_new() actually parsed the URI correctly. This is assuming soup_uri_new() actually returns NULL on error.
Review of attachment 308431 [details] [review]: Thanks for updating the patches. There are still 2 minor issues before this patch can be merged: 1) Your patch changes the file access mode from 644 to 755. I guess this was added by mistake. 2) After the patch you get a WARNING printed to the debug output when http_proxy="" saying that it couldn't be parsed. I guess making "" meaning no proxy shouldn't be considered a warning condition. I proposed considering "" as a correct input to the proxy and no warning should be printed.
Created attachment 308511 [details] [review] souphttpsrc: check null string before setting proxy Thank you for your reviews. I've uploaded new patch. 1) add checking empty string. 2) add considering the property setting to clear previous proxy. 3) modify the return of _src_set_proxy 4) about file access mode 5) not to display warning msg
Comment on attachment 308511 [details] [review] souphttpsrc: check null string before setting proxy Thanks for updating the patch: commit 8b6a26170346ccf5d01052229a0618d593b910c5 Author: Eunhae Choi <eunhae1.choi@samsung.com> Date: Thu Jul 30 11:29:27 2015 +0900 souphttpsrc: handle empty http proxy string 1) If the system http_proxy environment variable is not set or set to an empty string, we must not set proxy to avoid http connection error. 2) In case of proxy property setting, if user want to clear the proxy setting, they should be able to set it to NULL or an empty string again, so this is fixed too. 3) Check if the proxy string was parsed correctly. https://bugzilla.gnome.org/show_bug.cgi?id=752866
Well done Eunhae :)