[jira] [Commented] (QPID-3832) Qpid 0.14 broke transport connection setting

["jiraposter@reviews.apache.org (Commented) (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/QPID-3832?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13220020#comment-13220020 ] 

jiraposter@reviews.apache.org commented on QPID-3832:
-----------------------------------------------------


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4083/#review5495
-----------------------------------------------------------

Ship it!


makes sense to me -- that defaulting decision definitely belongs at higher level.

- mick


On 2012-02-28 15:51:40, Gordon Sim wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/4083/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2012-02-28 15:51:40)
bq.  
bq.  
bq.  Review request for Alan Conway, Kenneth Giusti and michael goulish.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Previous unification of URL support broke the ability to supply the 'transport' (e.g. tcp, rdma, ssl etc) on a connection. This can still be supplied in the URL itself if the 0-10 URL format is used but the regression should be fixed.
bq.  
bq.  The change in this patch adds a method to the Url class to parse a string without populating any resulting addresses with TCP as the default protocol if none is specified. This allows the protocol in the Address to indicate what was explicitly provided and lets the decision on defaults be taken elsewhere (in this case in the underlying client API). 
bq.  
bq.  
bq.  This addresses bug QPID-3832.
bq.      https://issues.apache.org/jira/browse/QPID-3832
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    /trunk/qpid/cpp/include/qpid/Url.h 1294525 
bq.    /trunk/qpid/cpp/src/qpid/Url.cpp 1294525 
bq.    /trunk/qpid/cpp/src/qpid/client/Connection.cpp 1294525 
bq.    /trunk/qpid/cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp 1294525 
bq.    /trunk/qpid/cpp/src/tests/ssl_test 1294525 
bq.  
bq.  Diff: https://reviews.apache.org/r/4083/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  New case added to sssl tests that uses a mixture of url and connection-option. Make check passes. 
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Gordon
bq.  
bq.


                
> Qpid 0.14 broke transport connection setting
> --------------------------------------------
>
>                 Key: QPID-3832
>                 URL: https://issues.apache.org/jira/browse/QPID-3832
>             Project: Qpid
>          Issue Type: Bug
>          Components: C++ Client
>    Affects Versions: 0.14
>            Reporter: Paul Colby
>            Assignee: Gordon Sim
>              Labels: SSL
>             Fix For: 0.15
>
>
> The transport connection setting was broken in r1156262.
> Consider the following test code:
> {code}
> #include <qpid/messaging/Connection.h>
> int main(int argc, char *argv[]) {
>     qpid::types::Variant::Map options;
>     options["transport" ] = "ssl";
>     qpid::messaging::Connection connection("localhost:5671", options);
>     connection.open();
>     return 0;
> }
> {code}
> Using Qpid 0.12 client libraries, the above code would use SSL.  But with Qpid 0.14, the code will *not* use SSL.
> The change in behaviour is a result of code changes to the {{ConnectionImpl::tryConnect}} function in {{cpp/src/qpid/client/amqp0_10/ConnectionImpl.cpp}} in r1156262
> Prior to that revision, the {{SimpleUrlParser}} was used, and {{settings}} passed to a {{Connection::open}} overload directly.  This worked fine.
> Post that revision, the code looks like this:
> {code}
> Url url(*i);
> if (url.getUser().size()) settings.username = url.getUser();
> if (url.getPass().size()) settings.password = url.getPass();
> QPID_LOG(debug, *i << " " << settings.protocol);
> QPID_LOG(debug, url);
> connection.open(url, settings);
> {code}
> (I added the {{QPID_LOG}} calls).
> The {{QPID_LOG}} calls produce output like:
> {code}
> localhost:5671 ssl
> amqp:tcp:localhost:5671
> {code}
> The problem is that the {{Url}} constructor is defaulting the protocol to TCP (via {{UrlParser::protocolAddr}}).  Then, when the {{Connection::open}} overload is called, the {{Url}} object's defaulted-but-now-explicit {{protocol}} value (TCP) is used in preference to the {{settings.protocol}} value (SSL).
> I think the correct solution here, thought it would be non-trivial, would be to pass {{settings}} (or at least {{settings.protocol}}) to the {{Url}} constructor as an optional default protocol.  Then the {{Url}} class should use that default protocol instead of TCP when the address string does not include an explicit protocol.
> Another option would be to provide some sort of {{Url::protocolWasDefaulted}} flag so that the {{ConnectionImpl::tryConnect}} function could override from {{settings}} when appropriate - this would be less code change, but the above solution would be more elegant IMO.
> Note, the obvious workaround is to always use explicit protocols in URLs.  That is, always use "ssl:hostname" instead of "hostname".

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project:      http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org