You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/12/20 03:45:34 UTC
Review Request 64736: Fixed conversion warnings.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/
-----------------------------------------------------------
Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
Repository: mesos
Description
-------
Fixed conversion warnings in `cram_md5` authentication. The potential
for overflow here is exceedingly unlikely, and we have been ignoring
these warnings to date without problem.
Fixed conversion warning in `files.cpp`. The signature accepts an
`off_t` type, but we calculated the offset from a `size_t`.
Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
`int`, so we changed the type.
Fixed conversion warnings in master. We can't change the return types
because the `Metric` class only accepts `double` types, so we
`static_cast` (as we were already doing elsewhere in the class).
Fixed warnings when converting to `pid_t` in the containerizer. Can't
change the arguments types because they're in the protocol, but they
obviously represent a `pid_t` originally, so this is a safe
`static_cast`.
Fixed warnings when constructing a `Seconds`a in `slave.cpp` as it takes
`int64_t`, but we're giving it a `double`. We can't change the `double`
because it's set in the protocol, and we're currently ignoring the
conversion anyway, so we just cast. We may want to provide a `double`
constructor for `Seconds` in the future that does proper rounding.
Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
was only used in `usage->mutable_exedcutors(i++)` which expected an
`int`, so we changed the type.
Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
functions take an `int`, and they're coming from a `size()`. We have
bigger problems if this overflows.
Diffs
-----
src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
src/master/master.hpp b800cda3c82ba7c471147889075cdc4f915f16b1
src/master/master.cpp cf8a22b636210e9f8d326592d8b64eab2c97caa4
src/master/registrar.cpp 488b0896b8bfbb02583d614376053bde35156dee
src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
src/slave/slave.cpp 55dea087acee299253ad1fac120e35653ca2c636
src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
Diff: https://reviews.apache.org/r/64736/diff/1/
Testing
-------
These are in Mesos.
Excluding 3rdparty:
On Windows 10:
```
Build succeeded.
0 Warning(s)
0 Error(s)
```
On CentOS 7, with Autotools:
```
make check
```
Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
Thanks,
Andrew Schwartzmeyer
Re: Review Request 64736: Fixed conversion warnings.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/#review194214
-----------------------------------------------------------
FAIL: Failed to apply the dependent review: 64730.
Failed command: `python.exe .\support\apply-reviews.py -n -r 64730`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64736
Relevant logs:
- [apply-review-64730-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64736/logs/apply-review-64730-stderr.log):
```
Traceback (most recent call last):
File ".\support\apply-reviews.py", line 434, in <module>
main()
File ".\support\apply-reviews.py", line 429, in main
reviewboard(options)
File ".\support\apply-reviews.py", line 419, in reviewboard
apply_review(options)
File ".\support\apply-reviews.py", line 158, in apply_review
fetch_patch(options)
File ".\support\apply-reviews.py", line 195, in fetch_patch
context=ssl_create_default_context())
File "C:\Python27\lib\urllib2.py", line 154, in urlopen
return opener.open(url, data, timeout)
File "C:\Python27\lib\urllib2.py", line 435, in open
response = meth(req, response)
File "C:\Python27\lib\urllib2.py", line 548, in http_response
'http', request, response, code, msg, hdrs)
File "C:\Python27\lib\urllib2.py", line 473, in error
return self._call_chain(*args)
File "C:\Python27\lib\urllib2.py", line 407, in _call_chain
result = func(*args)
File "C:\Python27\lib\urllib2.py", line 556, in http_error_default
raise HTTPError(req.get_full_url(), code, msg, hdrs, fp)
urllib2.HTTPError: HTTP Error 404: NOT FOUND
```
- Mesos Reviewbot Windows
On Dec. 20, 2017, 3:45 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64736/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2017, 3:45 a.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed conversion warnings in `cram_md5` authentication. The potential
> for overflow here is exceedingly unlikely, and we have been ignoring
> these warnings to date without problem.
>
> Fixed conversion warning in `files.cpp`. The signature accepts an
> `off_t` type, but we calculated the offset from a `size_t`.
>
> Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
> of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
> `int`, so we changed the type.
>
> Fixed conversion warnings in master. We can't change the return types
> because the `Metric` class only accepts `double` types, so we
> `static_cast` (as we were already doing elsewhere in the class).
>
> Fixed warnings when converting to `pid_t` in the containerizer. Can't
> change the arguments types because they're in the protocol, but they
> obviously represent a `pid_t` originally, so this is a safe
> `static_cast`.
>
> Fixed warnings when constructing a `Seconds`a in `slave.cpp` as it takes
> `int64_t`, but we're giving it a `double`. We can't change the `double`
> because it's set in the protocol, and we're currently ignoring the
> conversion anyway, so we just cast. We may want to provide a `double`
> constructor for `Seconds` in the future that does proper rounding.
>
> Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
> was only used in `usage->mutable_exedcutors(i++)` which expected an
> `int`, so we changed the type.
>
> Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
> functions take an `int`, and they're coming from a `size()`. We have
> bigger problems if this overflows.
>
>
> Diffs
> -----
>
> src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
> src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
> src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
> src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
> src/master/master.hpp b800cda3c82ba7c471147889075cdc4f915f16b1
> src/master/master.cpp cf8a22b636210e9f8d326592d8b64eab2c97caa4
> src/master/registrar.cpp 488b0896b8bfbb02583d614376053bde35156dee
> src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
> src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
> src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
> src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
> src/slave/slave.cpp 55dea087acee299253ad1fac120e35653ca2c636
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
>
> Diff: https://reviews.apache.org/r/64736/diff/1/
>
>
> Testing
> -------
>
> These are in Mesos.
>
> Excluding 3rdparty:
>
> On Windows 10:
>
> ```
> Build succeeded.
> 0 Warning(s)
> 0 Error(s)
> ```
>
> On CentOS 7, with Autotools:
>
> ```
> make check
> ```
>
> Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 64736: Fixed conversion warnings.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/#review195074
-----------------------------------------------------------
PASS: Mesos patch 64736 was successfully built and tested.
Reviews applied: `['64729', '64730', '64731', '64732', '64733', '64734', '64735', '64736']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64736
- Mesos Reviewbot Windows
On Jan. 9, 2018, 7:13 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64736/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2018, 7:13 p.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed conversion warnings in `cram_md5` authentication. The potential
> for overflow here is exceedingly unlikely, and we have been ignoring
> these warnings to date without problem.
>
> Fixed conversion warning in `files.cpp`. The signature accepts an
> `off_t` type, but we calculated the offset from a `size_t`.
>
> Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
> of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
> `int`, so we changed the type.
>
> Fixed conversion warnings in master. We can't change the return types
> because the `Metric` class only accepts `double` types, so we
> `static_cast` (as we were already doing elsewhere in the class).
>
> Fixed warnings when converting to `pid_t` in the containerizer. Can't
> change the arguments types because they're in the protocol, but they
> obviously represent a `pid_t` originally, so this is a safe
> `static_cast`.
>
> Fixed warnings when constructing a `Seconds`a in `slave.cpp` as it takes
> `int64_t`, but we're giving it a `double`. We can't change the `double`
> because it's set in the protocol, and we're currently ignoring the
> conversion anyway, so we just cast. We may want to provide a `double`
> constructor for `Seconds` in the future that does proper rounding.
>
> Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
> was only used in `usage->mutable_exedcutors(i++)` which expected an
> `int`, so we changed the type.
>
> Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
> functions take an `int`, and they're coming from a `size()`. We have
> bigger problems if this overflows.
>
>
> Diffs
> -----
>
> src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
> src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
> src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
> src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
> src/master/master.hpp 130f6e28cc62a8912aac66ecfbf014fe1ee444e3
> src/master/master.cpp 1610f4331225235e85f5a83e5338870cef99a5c8
> src/master/registrar.cpp ffb4582086aa939ffe7ccf4ff1d52fb68a9ecb78
> src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
> src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
> src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
> src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
> src/slave/slave.cpp aeb0fdaeda78f26de2ec790735bfa88c5be72850
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
>
> Diff: https://reviews.apache.org/r/64736/diff/3/
>
>
> Testing
> -------
>
> These are in Mesos.
>
> Excluding 3rdparty:
>
> On Windows 10:
>
> ```
> Build succeeded.
> 0 Warning(s)
> 0 Error(s)
> ```
>
> On CentOS 7, with Autotools:
>
> ```
> make check
> ```
>
> Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 64736: Fixed conversion warnings.
Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/#review195285
-----------------------------------------------------------
PASS: Mesos patch 64736 was successfully built and tested.
Reviews applied: `['64729', '64730', '64731', '64732', '64733', '64734', '64735', '64736']`
All the build artifacts available at: http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/64736
- Mesos Reviewbot Windows
On Jan. 12, 2018, 1:03 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64736/
> -----------------------------------------------------------
>
> (Updated Jan. 12, 2018, 1:03 a.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed conversion warnings in `cram_md5` authentication. The potential
> for overflow here is exceedingly unlikely, and we have been ignoring
> these warnings to date without problem.
>
> Fixed conversion warning in `files.cpp`. The signature accepts an
> `off_t` type, but we calculated the offset from a `size_t`.
>
> Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
> of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
> `int`, so we changed the type.
>
> Fixed conversion warnings in master. We can't change the return types
> because the `Metric` class only accepts `double` types, so we
> `static_cast` (as we were already doing elsewhere in the class).
>
> Fixed warnings when converting to `pid_t` in the containerizer. Can't
> change the arguments types because they're in the protocol, but they
> obviously represent a `pid_t` originally, so this is a safe
> `static_cast`.
>
> Fixed warnings when constructing a `Seconds` in `slave.cpp` as it takes
> `int64_t`, but we're giving it a `double`. We can't change the `double`
> because it's set in the protocol, and we're currently ignoring the
> conversion anyway, so we just cast. We may want to provide a `double`
> constructor for `Seconds` in the future that does proper rounding.
>
> Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
> was only used in `usage->mutable_executors(i++)` which expected an
> `int`, so we changed the type.
>
> Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
> functions take an `int`, and they're coming from a `size()`. We have
> bigger problems if this overflows.
>
>
> Diffs
> -----
>
> src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
> src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
> src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
> src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
> src/launcher/executor.cpp 794bf7ca07e68c7d83993546c134f85cac5a68e3
> src/master/master.hpp cdfd06ceb2a8eafa60d0af382b628b165e4ddcb5
> src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab
> src/master/registrar.cpp ffb4582086aa939ffe7ccf4ff1d52fb68a9ecb78
> src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
> src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
> src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
> src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
> src/slave/slave.cpp 0aa4e5e8e5965163ad44ead4dab92ed5ef2e2d8e
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
>
> Diff: https://reviews.apache.org/r/64736/diff/4/
>
>
> Testing
> -------
>
> These are in Mesos.
>
> Excluding 3rdparty:
>
> On Windows 10:
>
> ```
> Build succeeded.
> 0 Warning(s)
> 0 Error(s)
> ```
>
> On CentOS 7, with Autotools:
>
> ```
> make check
> ```
>
> Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 64736: Fixed conversion warnings.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/
-----------------------------------------------------------
(Updated Jan. 11, 2018, 5:03 p.m.)
Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
Changes
-------
Added a fix.
Repository: mesos
Description (updated)
-------
Fixed conversion warnings in `cram_md5` authentication. The potential
for overflow here is exceedingly unlikely, and we have been ignoring
these warnings to date without problem.
Fixed conversion warning in `files.cpp`. The signature accepts an
`off_t` type, but we calculated the offset from a `size_t`.
Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
`int`, so we changed the type.
Fixed conversion warnings in master. We can't change the return types
because the `Metric` class only accepts `double` types, so we
`static_cast` (as we were already doing elsewhere in the class).
Fixed warnings when converting to `pid_t` in the containerizer. Can't
change the arguments types because they're in the protocol, but they
obviously represent a `pid_t` originally, so this is a safe
`static_cast`.
Fixed warnings when constructing a `Seconds` in `slave.cpp` as it takes
`int64_t`, but we're giving it a `double`. We can't change the `double`
because it's set in the protocol, and we're currently ignoring the
conversion anyway, so we just cast. We may want to provide a `double`
constructor for `Seconds` in the future that does proper rounding.
Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
was only used in `usage->mutable_executors(i++)` which expected an
`int`, so we changed the type.
Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
functions take an `int`, and they're coming from a `size()`. We have
bigger problems if this overflows.
Diffs (updated)
-----
src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
src/launcher/executor.cpp 794bf7ca07e68c7d83993546c134f85cac5a68e3
src/master/master.hpp cdfd06ceb2a8eafa60d0af382b628b165e4ddcb5
src/master/master.cpp 8921964a3ed74538b8c831a2f827d07875c1a5ab
src/master/registrar.cpp ffb4582086aa939ffe7ccf4ff1d52fb68a9ecb78
src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
src/slave/slave.cpp 0aa4e5e8e5965163ad44ead4dab92ed5ef2e2d8e
src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
Diff: https://reviews.apache.org/r/64736/diff/4/
Changes: https://reviews.apache.org/r/64736/diff/3-4/
Testing
-------
These are in Mesos.
Excluding 3rdparty:
On Windows 10:
```
Build succeeded.
0 Warning(s)
0 Error(s)
```
On CentOS 7, with Autotools:
```
make check
```
Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
Thanks,
Andrew Schwartzmeyer
Re: Review Request 64736: Fixed conversion warnings.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/#review195089
-----------------------------------------------------------
Ship it!
In the commit description:
```
s/Secondsa/Seconds/
s/usage->mutable_exedcutors/usage->mutable_executors/
```
- Joseph Wu
On Jan. 9, 2018, 11:13 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64736/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2018, 11:13 a.m.)
>
>
> Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fixed conversion warnings in `cram_md5` authentication. The potential
> for overflow here is exceedingly unlikely, and we have been ignoring
> these warnings to date without problem.
>
> Fixed conversion warning in `files.cpp`. The signature accepts an
> `off_t` type, but we calculated the offset from a `size_t`.
>
> Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
> of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
> `int`, so we changed the type.
>
> Fixed conversion warnings in master. We can't change the return types
> because the `Metric` class only accepts `double` types, so we
> `static_cast` (as we were already doing elsewhere in the class).
>
> Fixed warnings when converting to `pid_t` in the containerizer. Can't
> change the arguments types because they're in the protocol, but they
> obviously represent a `pid_t` originally, so this is a safe
> `static_cast`.
>
> Fixed warnings when constructing a `Seconds`a in `slave.cpp` as it takes
> `int64_t`, but we're giving it a `double`. We can't change the `double`
> because it's set in the protocol, and we're currently ignoring the
> conversion anyway, so we just cast. We may want to provide a `double`
> constructor for `Seconds` in the future that does proper rounding.
>
> Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
> was only used in `usage->mutable_exedcutors(i++)` which expected an
> `int`, so we changed the type.
>
> Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
> functions take an `int`, and they're coming from a `size()`. We have
> bigger problems if this overflows.
>
>
> Diffs
> -----
>
> src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
> src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
> src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
> src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
> src/master/master.hpp 130f6e28cc62a8912aac66ecfbf014fe1ee444e3
> src/master/master.cpp 1610f4331225235e85f5a83e5338870cef99a5c8
> src/master/registrar.cpp ffb4582086aa939ffe7ccf4ff1d52fb68a9ecb78
> src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
> src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
> src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
> src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
> src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
> src/slave/slave.cpp aeb0fdaeda78f26de2ec790735bfa88c5be72850
> src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
>
>
> Diff: https://reviews.apache.org/r/64736/diff/3/
>
>
> Testing
> -------
>
> These are in Mesos.
>
> Excluding 3rdparty:
>
> On Windows 10:
>
> ```
> Build succeeded.
> 0 Warning(s)
> 0 Error(s)
> ```
>
> On CentOS 7, with Autotools:
>
> ```
> make check
> ```
>
> Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 64736: Fixed conversion warnings.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/
-----------------------------------------------------------
(Updated Jan. 9, 2018, 11:13 a.m.)
Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Fixed conversion warnings in `cram_md5` authentication. The potential
for overflow here is exceedingly unlikely, and we have been ignoring
these warnings to date without problem.
Fixed conversion warning in `files.cpp`. The signature accepts an
`off_t` type, but we calculated the offset from a `size_t`.
Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
`int`, so we changed the type.
Fixed conversion warnings in master. We can't change the return types
because the `Metric` class only accepts `double` types, so we
`static_cast` (as we were already doing elsewhere in the class).
Fixed warnings when converting to `pid_t` in the containerizer. Can't
change the arguments types because they're in the protocol, but they
obviously represent a `pid_t` originally, so this is a safe
`static_cast`.
Fixed warnings when constructing a `Seconds`a in `slave.cpp` as it takes
`int64_t`, but we're giving it a `double`. We can't change the `double`
because it's set in the protocol, and we're currently ignoring the
conversion anyway, so we just cast. We may want to provide a `double`
constructor for `Seconds` in the future that does proper rounding.
Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
was only used in `usage->mutable_exedcutors(i++)` which expected an
`int`, so we changed the type.
Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
functions take an `int`, and they're coming from a `size()`. We have
bigger problems if this overflows.
Diffs (updated)
-----
src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
src/master/master.hpp 130f6e28cc62a8912aac66ecfbf014fe1ee444e3
src/master/master.cpp 1610f4331225235e85f5a83e5338870cef99a5c8
src/master/registrar.cpp ffb4582086aa939ffe7ccf4ff1d52fb68a9ecb78
src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
src/slave/slave.cpp aeb0fdaeda78f26de2ec790735bfa88c5be72850
src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
Diff: https://reviews.apache.org/r/64736/diff/3/
Changes: https://reviews.apache.org/r/64736/diff/2-3/
Testing
-------
These are in Mesos.
Excluding 3rdparty:
On Windows 10:
```
Build succeeded.
0 Warning(s)
0 Error(s)
```
On CentOS 7, with Autotools:
```
make check
```
Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
Thanks,
Andrew Schwartzmeyer
Re: Review Request 64736: Fixed conversion warnings.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64736/
-----------------------------------------------------------
(Updated Jan. 5, 2018, 1:55 p.m.)
Review request for mesos, Greg Mann, Jie Yu, Joseph Wu, and Michael Park.
Changes
-------
Rebased.
Repository: mesos
Description
-------
Fixed conversion warnings in `cram_md5` authentication. The potential
for overflow here is exceedingly unlikely, and we have been ignoring
these warnings to date without problem.
Fixed conversion warning in `files.cpp`. The signature accepts an
`off_t` type, but we calculated the offset from a `size_t`.
Fixed `size_t` to `int` warning in `default_executor.cpp`. The only use
of this counter is in `taskGroup.tasks().Get(index++)`, which expects an
`int`, so we changed the type.
Fixed conversion warnings in master. We can't change the return types
because the `Metric` class only accepts `double` types, so we
`static_cast` (as we were already doing elsewhere in the class).
Fixed warnings when converting to `pid_t` in the containerizer. Can't
change the arguments types because they're in the protocol, but they
obviously represent a `pid_t` originally, so this is a safe
`static_cast`.
Fixed warnings when constructing a `Seconds`a in `slave.cpp` as it takes
`int64_t`, but we're giving it a `double`. We can't change the `double`
because it's set in the protocol, and we're currently ignoring the
conversion anyway, so we just cast. We may want to provide a `double`
constructor for `Seconds` in the future that does proper rounding.
Fixed `size_t` to `int` conversion warning in `slave.cpp`. This counter
was only used in `usage->mutable_exedcutors(i++)` which expected an
`int`, so we changed the type.
Fixed conversion warnings in `zookeeper.cpp`. The signatures of these
functions take an `int`, and they're coming from a `size()`. We have
bigger problems if this overflows.
Diffs (updated)
-----
src/authentication/cram_md5/authenticatee.cpp 7b3f767e29163de489018081089e67a6f22971c5
src/authentication/cram_md5/authenticator.cpp 2a12bb3f4aec79056ec6c82a8c896ee3af15bb4c
src/files/files.cpp ad2189b9f2b919eb2f70c738655fa6ecfaaa1b11
src/launcher/default_executor.cpp 6c88de413379d3b58f19a24ac0c2f43e38e85e54
src/master/master.hpp 130f6e28cc62a8912aac66ecfbf014fe1ee444e3
src/master/master.cpp 28d8be3a4769b418b61cff0b95845e4232135bc7
src/master/registrar.cpp ffb4582086aa939ffe7ccf4ff1d52fb68a9ecb78
src/slave/containerizer/mesos/containerizer.cpp cddc6173d17c8bd2585899b154f976c3822dffdd
src/slave/containerizer/mesos/isolators/posix.hpp caa282c95746e845992c971982892cf60c9b982c
src/slave/containerizer/mesos/isolators/windows/cpu.cpp 6b376efa9ff88ff93ee3dc8441fae4cda109d55f
src/slave/containerizer/mesos/isolators/windows/mem.cpp 64d11347d73328cb9665697b1ddd69a00276b51f
src/slave/containerizer/mesos/launcher.cpp ec31fa24c8b91583b8b327a0c658ed6e87bd292f
src/slave/slave.cpp cfb675df677e7a0d476b8d5a586afc2f197ab810
src/zookeeper/zookeeper.cpp 5ede4e5c47b30693ef9f296777dea8db2c4a48d8
Diff: https://reviews.apache.org/r/64736/diff/2/
Changes: https://reviews.apache.org/r/64736/diff/1-2/
Testing
-------
These are in Mesos.
Excluding 3rdparty:
On Windows 10:
```
Build succeeded.
0 Warning(s)
0 Error(s)
```
On CentOS 7, with Autotools:
```
make check
```
Passed. Also apparently -Wsign-compare is on, but not for Windows. Might re-evaluate our Windows warning level. I'd like to turn -Werror on, but all the compilers generate different sets of warnings :(
Thanks,
Andrew Schwartzmeyer