You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2019/01/05 23:07:39 UTC
Review Request 69673: Disallowed nan,
inf and so on when parsing Value::Scalar.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69673/
-----------------------------------------------------------
Review request for mesos and Benjamin Mahler.
Repository: mesos
Description
-------
Scalar values are intended to be finite numbers. This
patch checks `nan`, `inf` and so on when parsing
`Value::Scalar`. Only normal or zero numbers (as defined
in `std::fpclassify()`) are allowed.
Also added related tests.
Diffs
-----
src/common/values.cpp f04d115d2651da5a2784cb9e76757ecef9e2c118
src/tests/values_tests.cpp e4fcf982ac385f58e602eca78765f85485194e41
src/v1/values.cpp 1be99459ef8eb427b44f87234d64ebf099cd7866
Diff: https://reviews.apache.org/r/69673/diff/1/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 69673: Disallowed nan,
inf and so on when parsing Value::Scalar.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69673/
-----------------------------------------------------------
(Updated Jan. 6, 2019, 10:18 p.m.)
Review request for mesos and Benjamin Mahler.
Bugs: MESOS-9510
https://issues.apache.org/jira/browse/MESOS-9510
Repository: mesos
Description
-------
Scalar values are intended to be finite numbers. This
patch checks `nan`, `inf` and so on when parsing
`Value::Scalar`. Only normal or zero numbers (as defined
in `std::fpclassify()`) are allowed.
Also added related tests.
Diffs (updated)
-----
src/common/values.cpp f04d115d2651da5a2784cb9e76757ecef9e2c118
src/tests/values_tests.cpp e4fcf982ac385f58e602eca78765f85485194e41
src/v1/values.cpp 1be99459ef8eb427b44f87234d64ebf099cd7866
Diff: https://reviews.apache.org/r/69673/diff/3/
Changes: https://reviews.apache.org/r/69673/diff/2-3/
Testing
-------
make check
Thanks,
Meng Zhu
Re: Review Request 69673: Disallowed nan,
inf and so on when parsing Value::Scalar.
Posted by Meng Zhu <mz...@mesosphere.io>.
> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > Thanks for adding this validation! Looks good, just a suggestion for a clearer error message when the value is invalid
> >
> > Can you add a ticket for this?
Done. Linked to the patch.
> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > src/common/values.cpp
> > Lines 710-715 (patched)
> > <https://reviews.apache.org/r/69673/diff/2/?file=2117883#file2117883line710>
> >
> > We probably want to be a bit clearer about what's wrong with it rather than saying what not wrong is? I don't think the reader would know what "normal" is?
> >
> > E.g.
> >
> > ```
> > Option<Error> error = [=]() {
> > switch(std::fpclassify(*value_)) {
> > case FP_NORMAL: return None();
> > case FP_ZERO: return None();
> > case FP_INFINITE: return Error("Infinite values not supported");
> > case FP_NAN: return Error("NaN not supported)";
> > case FP_SUBNORMAL: return Error("Subnormal values not supported");
> > default: return Error("Unknown error");
> > }
> > }();
> >
> > if (error.isSome()) {
> > return Error("Invalid scalar value '" + temp + "'"
> > ": " + error->message);
> > }
> > ```
> >
> > btw we try to open and close the ' on the same line as the quoted item when possible to make it a bit clearer to the reader that the quoting is correct
Done. Thanks!
> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > src/common/values.cpp
> > Lines 715-721 (original), 722-728 (patched)
> > <https://reviews.apache.org/r/69673/diff/2/?file=2117883#file2117883line722>
> >
> > This case is actually pretty unfortunate.. if the user typos in "3.1a" it will fall into TEXT :(
I wonder do we have any use case for Value::TEXT atm? I guess we do not know... May be we could make it as deprecated in the proto and just throw an error here?
> On Jan. 6, 2019, 8:25 p.m., Benjamin Mahler wrote:
> > src/tests/values_tests.cpp
> > Lines 93-103 (patched)
> > <https://reviews.apache.org/r/69673/diff/2/?file=2117884#file2117884line93>
> >
> > You could group each pair without a newline? Do you also want to add a subnormal case?
Done.
- Meng
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69673/#review211712
-----------------------------------------------------------
On Jan. 6, 2019, 10:18 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69673/
> -----------------------------------------------------------
>
> (Updated Jan. 6, 2019, 10:18 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Bugs: MESOS-9510
> https://issues.apache.org/jira/browse/MESOS-9510
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Scalar values are intended to be finite numbers. This
> patch checks `nan`, `inf` and so on when parsing
> `Value::Scalar`. Only normal or zero numbers (as defined
> in `std::fpclassify()`) are allowed.
>
> Also added related tests.
>
>
> Diffs
> -----
>
> src/common/values.cpp f04d115d2651da5a2784cb9e76757ecef9e2c118
> src/tests/values_tests.cpp e4fcf982ac385f58e602eca78765f85485194e41
> src/v1/values.cpp 1be99459ef8eb427b44f87234d64ebf099cd7866
>
>
> Diff: https://reviews.apache.org/r/69673/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69673: Disallowed nan,
inf and so on when parsing Value::Scalar.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69673/#review211712
-----------------------------------------------------------
Fix it, then Ship it!
Thanks for adding this validation! Looks good, just a suggestion for a clearer error message when the value is invalid
Can you add a ticket for this?
src/common/values.cpp
Lines 710-715 (patched)
<https://reviews.apache.org/r/69673/#comment297236>
We probably want to be a bit clearer about what's wrong with it rather than saying what not wrong is? I don't think the reader would know what "normal" is?
E.g.
```
Option<Error> error = [=]() {
switch(std::fpclassify(*value_)) {
case FP_NORMAL: return None();
case FP_ZERO: return None();
case FP_INFINITE: return Error("Infinite values not supported");
case FP_NAN: return Error("NaN not supported)";
case FP_SUBNORMAL: return Error("Subnormal values not supported");
default: return Error("Unknown error");
}
}();
if (error.isSome()) {
return Error("Invalid scalar value '" + temp + "'"
": " + error->message);
}
```
btw we try to open and close the ' on the same line as the quoted item when possible to make it a bit clearer to the reader that the quoting is correct
src/common/values.cpp
Lines 715-721 (original), 722-728 (patched)
<https://reviews.apache.org/r/69673/#comment297237>
This case is actually pretty unfortunate.. if the user typos in "3.1a" it will fall into TEXT :(
src/tests/values_tests.cpp
Lines 93-103 (patched)
<https://reviews.apache.org/r/69673/#comment297238>
You could group each pair without a newline? Do you also want to add a subnormal case?
src/v1/values.cpp
Lines 738-743 (patched)
<https://reviews.apache.org/r/69673/#comment297239>
Ditto here.
- Benjamin Mahler
On Jan. 5, 2019, 11:49 p.m., Meng Zhu wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69673/
> -----------------------------------------------------------
>
> (Updated Jan. 5, 2019, 11:49 p.m.)
>
>
> Review request for mesos and Benjamin Mahler.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Scalar values are intended to be finite numbers. This
> patch checks `nan`, `inf` and so on when parsing
> `Value::Scalar`. Only normal or zero numbers (as defined
> in `std::fpclassify()`) are allowed.
>
> Also added related tests.
>
>
> Diffs
> -----
>
> src/common/values.cpp f04d115d2651da5a2784cb9e76757ecef9e2c118
> src/tests/values_tests.cpp e4fcf982ac385f58e602eca78765f85485194e41
> src/v1/values.cpp 1be99459ef8eb427b44f87234d64ebf099cd7866
>
>
> Diff: https://reviews.apache.org/r/69673/diff/2/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Meng Zhu
>
>
Re: Review Request 69673: Disallowed nan,
inf and so on when parsing Value::Scalar.
Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69673/
-----------------------------------------------------------
(Updated Jan. 5, 2019, 3:49 p.m.)
Review request for mesos and Benjamin Mahler.
Changes
-------
Added more tests.
Repository: mesos
Description
-------
Scalar values are intended to be finite numbers. This
patch checks `nan`, `inf` and so on when parsing
`Value::Scalar`. Only normal or zero numbers (as defined
in `std::fpclassify()`) are allowed.
Also added related tests.
Diffs (updated)
-----
src/common/values.cpp f04d115d2651da5a2784cb9e76757ecef9e2c118
src/tests/values_tests.cpp e4fcf982ac385f58e602eca78765f85485194e41
src/v1/values.cpp 1be99459ef8eb427b44f87234d64ebf099cd7866
Diff: https://reviews.apache.org/r/69673/diff/2/
Changes: https://reviews.apache.org/r/69673/diff/1-2/
Testing
-------
make check
Thanks,
Meng Zhu