RE: [Patch] Proxy command-line argument
Well, after some very minor refactoring and reformatting, here's what I will commit (in twelve hours or so, unless anyone raises any issue between now and then). It builds cleanly and I tested that it works by installing a couple of packages through Tor[*] and found no problems. Thanks again to Vincent for the patch and Warren for C++ advice :) cheers, DaveK [*] - . OR DID I ?!!!1!!! MUAHAHHAHAAA!!! -- Can't think of a witty .sigline today setup-proxy-option-patch.diff Description: Binary data
Re: [Patch] Proxy command-line argument
Is the patch OK ? Vincent 2008/1/28, Dave Korn [EMAIL PROTECTED]: On 28 January 2008 09:45, Corinna Vinschen wrote: Is any of you setup guys going to look into this patch? Brian? Dave? Igor? I'll have a look at it after work this evening if nobody else gets to it first. cheers, DaveK -- Can't think of a witty .sigline today
RE: [Patch] Proxy command-line argument
On 31 January 2008 08:05, Vincent Privat wrote:
Before we even get started: Thanks for your contribution! We're pretty
under resourced here and every little bit really helps :)
Is the patch OK ?
It patched and compiles fine. I haven't had time to test it yet, but it
looks fairly obvious so I expect it'll work fine. Couple of questions:
+ std::string proxyString(((std::string)ProxyOption).c_str());
I'm not a C++ expert, so maybe I'm missing something, but if I read that
right you're taking ProxyOption (type StringOption), calling the std::string
conversion operator to return a std::string, getting a const char* from that
and using it to build a std::string. Why the trip via c_str? Can't you just
construct the new std::string from the return of the conversion operator
directly by invoking the std::string copy c-tor?
+ std::string proxyString((std::string)ProxyOption);
That seems simpler to me, but like I said I may be overlooking something.
+ if (proxyString.size())
+ {
+NetIO::net_method = IDC_NET_PROXY;
If parsing the string fails now, we'll be in an invalid state later won't
we?
+unsigned int pos = proxyString.find_last_of(':');
+if (pos = 0)
+{
+ NetIO::net_proxy_host = strdup(proxyString.substr(0, pos).c_str());
+ if (pos proxyString.size()-1)
+ {
+std::string portString = proxyString.substr(pos+1,
proxyString.size()-(pos+1));
+std::istringstream iss(portString, std::istringstream::in);
+iss NetIO::net_proxy_port;
Do we really need to go to the lengths of building an istringstream here?
Wouldn't good old atoi be simpler? ;-)
+ }
+}
+ }
The only one of these that I think actually matters is the assignment to
NetIO::net_method, which I think we should move inside the if (pos = 0);
the rest I'm just curious about. I'll probably finish testing and commit it
at the weekend.
cheers,
DaveK
--
Can't think of a witty .sigline today
Re: [Patch] Proxy command-line argument
Dave Korn wrote: + std::string proxyString(((std::string)ProxyOption).c_str()); I'm not a C++ expert, so maybe I'm missing something, but if I read that right you're taking ProxyOption (type StringOption), calling the std::string conversion operator to return a std::string, getting a const char* from that and using it to build a std::string. Your non-expert eyes have seen aright. It is either circumlocutious, or broken. If ProxyOption has operator std::string, this does the same thing: std::string proxyString(ProxyOption); If it doesn't, the code will still compile because he's using a brute-force C style cast here. But, it would almost certainly crash. Actually, there's a third possibility: the original author needs to truncate the string at the first embedded null character. The direct std::string copy will preserve a null. + std::string proxyString((std::string)ProxyOption); Just to reinforce the point, the cast is either unhelpful or harmful. If you really must force the conversion, use one of the new-style C++ casts to limit the amount of damage.
Re: [Patch] Proxy command-line argument
On Jan 21 09:02, Vincent Privat wrote: On Mon, Jan 21, 2008 at 12:19:56AM -0500, Christopher Faylor wrote: Please provide this as an actual patch: http://en.wikipedia.org/wiki/Patch_%28Unix%29 A patch is basically just a diff -du old new. Sending the whole file makes it hard to figure out what you've changed. If possible, a patch against the current CVS sources is preferred. See: http://sourceware.org/cygwin-apps/setup.html cgf Thanks ! I attached this time a good patch file changelog :) Vincent Is any of you setup guys going to look into this patch? Brian? Dave? Igor? Corinna -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Project Co-Leader cygwin AT cygwin DOT com Red Hat
RE: [Patch] Proxy command-line argument
On 28 January 2008 09:45, Corinna Vinschen wrote: Is any of you setup guys going to look into this patch? Brian? Dave? Igor? I'll have a look at it after work this evening if nobody else gets to it first. cheers, DaveK -- Can't think of a witty .sigline today
Re: [Patch] Proxy command-line argument
On Mon, Jan 21, 2008 at 12:19:56AM -0500, Christopher Faylor wrote: Please provide this as an actual patch: http://en.wikipedia.org/wiki/Patch_%28Unix%29 A patch is basically just a diff -du old new. Sending the whole file makes it hard to figure out what you've changed. If possible, a patch against the current CVS sources is preferred. See: http://sourceware.org/cygwin-apps/setup.html cgf Thanks ! I attached this time a good patch file changelog :) Vincent net.cc.diff Description: Binary data net.cc.changelog Description: Binary data
Re: [Patch] Proxy command-line argument
On Mon, Jan 21, 2008 at 12:19:56AM -0500, Christopher Faylor wrote: On Mon, Jan 21, 2008 at 02:38:35AM +0100, Vincent Privat wrote: The only modified file is net.cc, which is attached to this mail. Basically, the modifications are: - Inclusion of sstream and getopt++/StringOption.h - Addition of static StringOption ProxyOption (, 'p', proxy, HTTP/FTP proxy (host:port), false); - Modification of NetPage::OnInit () method to modify NetIO::net_method , NetIO::net_proxy_host and NetIO::net_proxy_port I think my changes are conformed to the Cygwin coding guidelines and I hope to see this patch accepted. As it is my very first patch, I'm not sure if I must send it to this mailing-list; or to the cygwin-patches one ? Since cygwin-patches says this: * cygwin-patches: a list for submitting patches to the Cygwin DLL and the other components of the winsup directory (if you aren't sure what this means, then you shouldn't be sending email here). Discussions of supplied patches are also acceptable, of course. Only subscribers may submit email to this list. that should narrow down your choice to either this mailing list or the cygwin mailing list or cygwin-apps and cygwin-apps is the correct choice. cgf
Re: [Patch] Proxy command-line argument
On Mon, Jan 21, 2008 at 02:38:35AM +0100, Vincent Privat wrote: I made a small patch to setup.exe. It adds the following command line argument: -p --proxy HTTP/FTP proxy (host:port) Thanks! The only modified file is net.cc, which is attached to this mail. Basically, the modifications are: - Inclusion of sstream and getopt++/StringOption.h - Addition of static StringOption ProxyOption (, 'p', proxy, HTTP/FTP proxy (host:port), false); - Modification of NetPage::OnInit () method to modify NetIO::net_method , NetIO::net_proxy_host and NetIO::net_proxy_port I think my changes are conformed to the Cygwin coding guidelines and I hope to see this patch accepted. As it is my very first patch, I'm not sure if I must send it to this mailing-list; or to the cygwin-patches one ? Since cygwin-patches says this: * cygwin-patches: a list for submitting patches to the Cygwin DLL and the other components of the winsup directory (if you aren't sure what this means, then you shouldn't be sending email here). Discussions of supplied patches are also acceptable, of course. Only subscribers may submit email to this list. that should narrow down your choice to either this mailing list or cygwin-apps and cygwin-apps is the correct choice. Thanks in advance for your comments. Please provide this as an actual patch: http://en.wikipedia.org/wiki/Patch_%28Unix%29 A patch is basically just a diff -du old new. Sending the whole file makes it hard to figure out what you've changed. If possible, a patch against the current CVS sources is preferred. See: http://sourceware.org/cygwin-apps/setup.html cgf
