qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Stitcher <astitc...@apache.org>
Subject Re: Review Request 45340: turn off async ANONYMOUS SASL negotiation by default
Date Mon, 28 Mar 2016 15:34:16 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message