mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 37097: Fix 'Accept-Encoding' parsing
Date Tue, 11 Aug 2015 19:07:05 GMT

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

Ship it!


Couple of comments below, but I'll make the adjustments and get this committed for you shortly!
Thanks for splitting this out and updating the test!


3rdparty/libprocess/include/process/http.hpp (line 120)
<https://reviews.apache.org/r/37097/#comment149686>

    We're still wrapping comments at 70 for now, but that might change soon :)



3rdparty/libprocess/include/process/http.hpp (line 122)
<https://reviews.apache.org/r/37097/#comment149684>

    Which RFC? :)



3rdparty/libprocess/include/process/http.hpp (lines 124 - 125)
<https://reviews.apache.org/r/37097/#comment149683>

    This bit seems to just be re-iterating the summary above?



3rdparty/libprocess/src/http.cpp (lines 134 - 135)
<https://reviews.apache.org/r/37097/#comment149692>

    Reading through the RFC again, it's quite a bit more subtle than this:
    
    ```
    4. The "identity" content-coding is always acceptable, unless
       specifically refused because the Accept-Encoding field includes
       "identity;q=0", or because the field includes "*;q=0" and does
       not explicitly include the "identity" content-coding. If the
       Accept-Encoding field-value is empty, then only the "identity"
       encoding is acceptable.
    
    If no Accept-Encoding field is present in a request, the server MAY assume that the client
will accept any content coding. In this case, if "identity" is one of the available content-codings,
then the server SHOULD use the "identity" content-coding, unless it has additional information
that a different content-coding is meaningful to the client.
    
          Note: If the request does not include an Accept-Encoding field,
          and if the "identity" content-coding is unavailable, then
          content-codings commonly understood by HTTP/1.0 clients (i.e.,
          "gzip" and "compress") are preferred; some older clients
          improperly display messages sent with other content-codings.  The
          server might also make this decision based on information about
          the particular user-agent or client.
          Note: Most HTTP/1.0 applications do not recognize or obey qvalues
          associated with content-codings. This means that qvalues will not
          work and are not permitted with x-gzip or x-compress.
    ```
    
    So it appears that we're doing the right thing here by returning false and using the identity
encoding, but we don't correctly deal with explicitly rejected identity encoding for now..
I'll remove this and the TODO since it's a bit misleading



3rdparty/libprocess/src/http.cpp (lines 145 - 147)
<https://reviews.apache.org/r/37097/#comment149688>

    Shouldn't this be up where we're returning false..?



3rdparty/libprocess/src/http.cpp (line 159)
<https://reviews.apache.org/r/37097/#comment149701>

    Hm.. it's a bit implicit that tokens.empty is guaranteed to be false here because 'encoding'
itself is non-empty due to it coming from tokenize.


- Ben Mahler


On Aug. 7, 2015, 6:52 p.m., Isabel Jimenez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37097/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2015, 6:52 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently parsing only compares the begining of the header making 'gzipbug' match with
candidate 'gzip'
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/include/process/http.hpp b8d9300 
>   3rdparty/libprocess/src/http.cpp 4dcbd74 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 0032137 
> 
> Diff: https://reviews.apache.org/r/37097/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>


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