Review Request 45340: turn off async ANONYMOUS SASL negotiation by default

[Cliff Jansen <cl...@gmail.com>]

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

Review request for qpid.


Bugs: PROTON-1135
    https://issues.apache.org/jira/browse/PROTON-1135


Repository: qpid-proton-git


Description
-------

Add new sasl api methods:

  pn_sasl_set_async_negotiation
  pn_sasl_get_async_negotiation
  
Turn async off by default

Add python test.


Diffs
-----

  proton-c/bindings/python/proton/__init__.py 5ffede8 
  proton-c/include/proton/sasl.h 354982f 
  proton-c/src/sasl/sasl-internal.h b3f4c7f 
  proton-c/src/sasl/sasl.c 29d377e 
  tests/python/proton_tests/engine.py 0a6eb8d 
  tests/python/proton_tests/sasl.py 6adb77d 

Diff: https://reviews.apache.org/r/45340/diff/


Testing
-------

linux ctest


Thanks,

Cliff Jansen

Re: Review Request 45340: turn off async ANONYMOUS SASL negotiation by default

[Andrew Stitcher <as...@apache.org>]

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



NAK.

The implementation is fundamentally wrong.

But, I think this change is wrong headed, because it is my view that pipelining is in the protocol spec, and is a useful feature (in admittedly very limited circumstances)

However if you want to get rid of for interop reasons then there is no reason for a switch - a user can *never* know to turn it on as they can'tbe sure of the implementation at the other end.

The bigger issue is that peer implementations won't get fixed to accept pipeling and this will inhibit very small AMQP implementations from being usable because they won't interoperate.


proton-c/src/sasl/sasl.c (line 363)
<https://reviews.apache.org/r/45340/#comment188517>

    You can't do this here.
    
    It breaks the design of the code.
    
    There are pn_output_write_*_header() routines in several places and their purpose is only to copy bytes around (for the appropriate protocol header).
    
    Further pni_sasl_force_anonymous() is designed to happen on the input side as it changes the state machine. This change calls it on the output side.



proton-c/src/sasl/sasl.c (line 652)
<https://reviews.apache.org/r/45340/#comment188518>

    I think adding more inscrutable booleans is a bad idea.
    
    If we really need to stop pipelining. Then just stop doing it, there is no reasonable way a user can know to turn it on or off. It is part of the protocol so turning it on is purely an interop work around.
    
    But presumable there is no way to know who is at the other end, so there is no way to ever turn it on if we suspect that some peer implementations may be broken.


- Andrew Stitcher


On March 25, 2016, 7:16 p.m., Cliff Jansen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45340/
> -----------------------------------------------------------
> 
> (Updated March 25, 2016, 7:16 p.m.)
> 
> 
> Review request for qpid.
> 
> 
> Bugs: PROTON-1135
>     https://issues.apache.org/jira/browse/PROTON-1135
> 
> 
> Repository: qpid-proton-git
> 
> 
> Description
> -------
> 
> Add new sasl api methods:
> 
>   pn_sasl_set_async_negotiation
>   pn_sasl_get_async_negotiation
>   
> Turn async off by default
> 
> Add python test.
> 
> 
> Diffs
> -----
> 
>   proton-c/bindings/python/proton/__init__.py 5ffede8 
>   proton-c/include/proton/sasl.h 354982f 
>   proton-c/src/sasl/sasl-internal.h b3f4c7f 
>   proton-c/src/sasl/sasl.c 29d377e 
>   tests/python/proton_tests/engine.py 0a6eb8d 
>   tests/python/proton_tests/sasl.py 6adb77d 
> 
> Diff: https://reviews.apache.org/r/45340/diff/
> 
> 
> Testing
> -------
> 
> linux ctest
> 
> 
> Thanks,
> 
> Cliff Jansen
> 
>