[zypp-devel] [PATCH 2/2] Add support for using libproxy to find proxies
This one probably isn't ready to apply as-is, but it's working for me
right now. Problems include:
- it should have proper CMake magic to find and build against (and
optionally *not* build against) libproxy.
- it probably shouldn't unconditionally subvert the old code which
looks in /etc/sysconfig/proxy for per-url-scheme information.
- There's a memory leak when we return a proxy specified by libproxy.
I figured that all of those were probably best handled by someone who
knows the code (or just knows CMake/C++) better than me.
---
zypp/CMakeLists.txt | 3 +
zypp/media/MediaCurl.cc | 2 +-
zypp/media/proxyinfo/ProxyInfoLibproxy.cc | 87 +++++++++++++++++++++++++++++
zypp/media/proxyinfo/ProxyInfoLibproxy.h | 57 +++++++++++++++++++
zypp/media/proxyinfo/ProxyInfos.h | 1 +
5 files changed, 149 insertions(+), 1 deletions(-)
create mode 100644 zypp/media/proxyinfo/ProxyInfoLibproxy.cc
create mode 100644 zypp/media/proxyinfo/ProxyInfoLibproxy.h
diff --git a/zypp/CMakeLists.txt b/zypp/CMakeLists.txt
index d4f4682..796968f 100644
--- a/zypp/CMakeLists.txt
+++ b/zypp/CMakeLists.txt
@@ -326,11 +326,13 @@ INSTALL( FILES
SET( zypp_media_proxyinfo_SRCS
media/proxyinfo/ProxyInfoSysconfig.cc
+ media/proxyinfo/ProxyInfoLibproxy.cc
)
SET( zypp_media_proxyinfo_HEADERS
media/proxyinfo/ProxyInfoImpl.h
media/proxyinfo/ProxyInfoSysconfig.h
+ media/proxyinfo/ProxyInfoLibproxy.h
media/proxyinfo/ProxyInfos.h
)
@@ -912,6 +914,7 @@ TARGET_LINK_LIBRARIES(zypp ${OPENSSL_LIBRARIES} )
TARGET_LINK_LIBRARIES(zypp ${CRYPTO_LIBRARIES} )
TARGET_LINK_LIBRARIES(zypp ${SIGNALS_LIBRARY} )
TARGET_LINK_LIBRARIES(zypp ${UDEV_LIBRARY} )
+TARGET_LINK_LIBRARIES(zypp -lproxy )
INSTALL(TARGETS zypp LIBRARY DESTINATION ${LIB_INSTALL_DIR} )
diff --git a/zypp/media/MediaCurl.cc b/zypp/media/MediaCurl.cc
index bf06908..b0c6635 100644
--- a/zypp/media/MediaCurl.cc
+++ b/zypp/media/MediaCurl.cc
@@ -326,7 +326,7 @@ void fillSettingsFromUrl( const Url &url, TransferSettings &s )
*/
void fillSettingsSystemProxy( const Url&url, TransferSettings &s )
{
- ProxyInfo proxy_info (ProxyInfo::ImplPtr(new ProxyInfoSysconfig("proxy")));
+ ProxyInfo proxy_info (ProxyInfo::ImplPtr(new ProxyInfoLibproxy()));
s.setProxyEnabled( proxy_info.useProxyFor( url ) );
if ( s.proxyEnabled() )
s.setProxy(proxy_info.proxy(url));
diff --git a/zypp/media/proxyinfo/ProxyInfoLibproxy.cc b/zypp/media/proxyinfo/ProxyInfoLibproxy.cc
new file mode 100644
index 0000000..ef7bdac
--- /dev/null
+++ b/zypp/media/proxyinfo/ProxyInfoLibproxy.cc
@@ -0,0 +1,87 @@
+/*---------------------------------------------------------------------\
+| ____ _ __ __ ___ |
+| |__ / \ / / . \ . \ |
+| / / \ V /| _/ _/ |
+| / /__ | | | | | | |
+| /_____||_| |_| |_| |
+| |
+\---------------------------------------------------------------------*/
+/** \file zypp/media/proxyinfo/ProxyInfoLibproxy.cc
+ *
+*/
+
+#include <iostream>
+#include <fstream>
+
+#include "zypp/base/Logger.h"
+#include "zypp/base/String.h"
+#include "zypp/Pathname.h"
+
+#include "zypp/media/proxyinfo/ProxyInfoLibproxy.h"
+
+using namespace std;
+using namespace zypp::base;
+
+namespace zypp {
+ namespace media {
+
+ ProxyInfoLibproxy::ProxyInfoLibproxy()
+ : ProxyInfo::Impl()
+ {
+ _factory = px_proxy_factory_new();
+ _enabled = !(_factory == NULL);
+ }
+
+ ProxyInfoLibproxy::~ProxyInfoLibproxy()
+ {
+ if (_enabled) {
+ px_proxy_factory_free(_factory);
+ _factory = NULL;
+ _enabled = false;
+ }
+ }
+
+ std::string ProxyInfoLibproxy::proxy(const Url & url_r) const
+ {
+ if (!_enabled)
+ return "";
+
+ const url::ViewOption vopt =
+ url::ViewOption::WITH_SCHEME
+ + url::ViewOption::WITH_HOST
+ + url::ViewOption::WITH_PORT
+ + url::ViewOption::WITH_PATH_NAME;
+
+ char **proxies = px_proxy_factory_get_proxies(_factory,
+ (char *)url_r.asString(vopt).c_str());
+ if (!proxies)
+ return "";
+
+ /* cURL can only handle HTTP proxies, not SOCKS. And can only handle
+ one. So look through the list and find an appropriate one. */
+ char *result = NULL;
+
+ for (int i = 0; proxies[i]; i++) {
+ if (!result &&
+ !strncmp(proxies[i], "http://", 7))
+ result = proxies[i];
+ else
+ free(proxies[i]);
+ }
+ free(proxies);
+
+ if (!result)
+ return "";
+
+ /* Yes, there's a memory leak here */
+ return result;
+ }
+
+ ProxyInfo::NoProxyIterator ProxyInfoLibproxy::noProxyBegin() const
+ { return _no_proxy.begin(); }
+
+ ProxyInfo::NoProxyIterator ProxyInfoLibproxy::noProxyEnd() const
+ { return _no_proxy.end(); }
+
+ } // namespace media
+} // namespace zypp
diff --git a/zypp/media/proxyinfo/ProxyInfoLibproxy.h b/zypp/media/proxyinfo/ProxyInfoLibproxy.h
new file mode 100644
index 0000000..b181064
--- /dev/null
+++ b/zypp/media/proxyinfo/ProxyInfoLibproxy.h
@@ -0,0 +1,57 @@
+/*---------------------------------------------------------------------\
+| ____ _ __ __ ___ |
+| |__ / \ / / . \ . \ |
+| / / \ V /| _/ _/ |
+| / /__ | | | | | | |
+| /_____||_| |_| |_| |
+| |
+\---------------------------------------------------------------------*/
+/** \file zypp/media/proxyinfo/ProxyInfoLibproxy.h
+ *
+*/
+#ifndef ZYPP_MEDIA_PROXYINFO_PROXYINFOLIBPROXY_H
+#define ZYPP_MEDIA_PROXYINFO_PROXYINFOLIBPROXY_H
+
+#include <string>
+#include <map>
+
+#include
On 11/20/2010 02:16 PM, David Woodhouse wrote:
This one probably isn't ready to apply as-is, but it's working for me right now. Problems include:
- it should have proper CMake magic to find and build against (and optionally *not* build against) libproxy.
- it probably shouldn't unconditionally subvert the old code which looks in /etc/sysconfig/proxy for per-url-scheme information.
- There's a memory leak when we return a proxy specified by libproxy.
David, thanks a lot for the patches I have applied the first one locally as is. It only changes the API to do it more powerful, but the behavior is basically unchanged. (and thanks a lot for separating the patches in that logical way) For the second one, I added a real check for libproxy, and compiled the files conditionally. If libproxy is found, then it is used, otherwise the default Sysconfig one is used. May be we should remove the Sysconfig one completely then. However once I build, the testcases fail. The following tests FAILED: 12 - Fetcher_test (Failed) 17 - MediaSetAccess_test (Failed) 24 - RepoInfo_test (Failed) 48 - MirrorList_test (Failed) Errors while running CTest -- Duncan Mac-Vicar P. - Novell® Making IT Work As One™ SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg)
On Mon, 2010-11-22 at 12:50 +0100, Duncan Mac-Vicar P. wrote:
On 11/20/2010 02:16 PM, David Woodhouse wrote:
This one probably isn't ready to apply as-is, but it's working for me right now. Problems include:
- it should have proper CMake magic to find and build against (and optionally *not* build against) libproxy.
- it probably shouldn't unconditionally subvert the old code which looks in /etc/sysconfig/proxy for per-url-scheme information.
- There's a memory leak when we return a proxy specified by libproxy.
That memory leak is probably fixed by this, FWIW: --- a/zypp/media/proxyinfo/ProxyInfoLibproxy.cc +++ b/zypp/media/proxyinfo/ProxyInfoLibproxy.cc @@ -73,8 +73,9 @@ namespace zypp { if (!result) return ""; - /* Yes, there's a memory leak here */ - return result; + std::string sresult = result; + free(result); + return sresult; } ProxyInfo::NoProxyIterator ProxyInfoLibproxy::noProxyBegin() const
David, thanks a lot for the patches
I have applied the first one locally as is. It only changes the API to do it more powerful, but the behavior is basically unchanged. (and thanks a lot for separating the patches in that logical way)
For the second one, I added a real check for libproxy, and compiled the files conditionally. If libproxy is found, then it is used, otherwise the default Sysconfig one is used. May be we should remove the Sysconfig one completely then.
I may have done that the other way round. If there's a hard-coded proxy (be it in /etc/sysconfig/proxy or $http_proxy) then we probably want to use that in *preference* to the automatically detected one from libproxy. The user should always be able to override.
However once I build, the testcases fail.
The following tests FAILED: 12 - Fetcher_test (Failed) 17 - MediaSetAccess_test (Failed) 24 - RepoInfo_test (Failed) 48 - MirrorList_test (Failed) Errors while running CTest
Hrm, that's interesting. Of course, you are now relying on libproxy to return the *correct* information about proxies. Is it? -- dwmw2 -- To unsubscribe, e-mail: zypp-devel+unsubscribe@opensuse.org For additional commands, e-mail: zypp-devel+help@opensuse.org
On Monday 22 November 2010 12:59:34 David Woodhouse wrote:
--- a/zypp/media/proxyinfo/ProxyInfoLibproxy.cc +++ b/zypp/media/proxyinfo/ProxyInfoLibproxy.cc @@ -73,8 +73,9 @@ namespace zypp { if (!result) return "";
- /* Yes, there's a memory leak here */ - return result; + std::string sresult = result; + free(result); + return sresult; }
I' vote for an undelayed 'free()': /* cURL can only handle HTTP proxies, not SOCKS. And can only handle one. So look through the list and find an appropriate one. */ - char *result = NULL; + std::string result; for (int i = 0; proxies[i]; i++) { - if (!result && + if (result.empty() && !strncmp(proxies[i], "http://", 7)) result = proxies[i]; - else - free(proxies[i]); + free(proxies[i]); - } free(proxies); - if (!result) - return ""; - - /* Yes, there's a memory leak here */ return result; } -- cu, Michael Andres +------------------------------------------------------------------+ Key fingerprint = 2DFA 5D73 18B1 E7EF A862 27AC 3FB8 9E3A 27C6 B0E4 +------------------------------------------------------------------+ Michael Andres ZYPP Development ma@suse.de SUSE LINUX Products GmbH, GF: Markus Rex, HRB 16746 (AG Nuernberg) Maxfeldstrasse 5, D-90409 Nuernberg, Germany, ++49 (0)911 - 740 53-0 +------------------------------------------------------------------+ -- To unsubscribe, e-mail: zypp-devel+unsubscribe@opensuse.org For additional commands, e-mail: zypp-devel+help@opensuse.org
I am tracking this now on: https://bugzilla.novell.com/show_bug.cgi?id=655483 -- To unsubscribe, e-mail: zypp-devel+unsubscribe@opensuse.org For additional commands, e-mail: zypp-devel+help@opensuse.org
On Mon, 2010-11-22 at 12:50 +0100, Duncan Mac-Vicar P. wrote:
For the second one, I added a real check for libproxy, and compiled the files conditionally. If libproxy is found, then it is used, otherwise the default Sysconfig one is used. May be we should remove the Sysconfig one completely then.
However once I build, the testcases fail.
The following tests FAILED: 12 - Fetcher_test (Failed) 17 - MediaSetAccess_test (Failed) 24 - RepoInfo_test (Failed) 48 - MirrorList_test (Failed) Errors while running CTest
I believe you established that this was a problem with your local
libproxy implementation? Certainly the tests are passing fine for me.
Can we merge the code from the libproxy branch into master? Probably
worth adding this patch, while you're at it...
commit d1cb8915d571fe6d9ecf41db60c7184d6ede3e9c
Author: David Woodhouse
participants (3)
-
David Woodhouse
-
Duncan Mac-Vicar P.
-
Michael Andres