You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by zhou xing <xi...@cn.ibm.com> on 2016/04/19 10:07:05 UTC
Review Request 46373: Request /files/read.json with a negative length
value causes error.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/
-----------------------------------------------------------
Review request for mesos, Ben Mahler and Greg Mann.
Bugs: mesos-5060
https://issues.apache.org/jira/browse/mesos-5060
Repository: mesos
Description
-------
[MESOS-5060]
The patch did the following changes:
1. Fix the offset and length logic in files.cpp.
2. Add some tests to test the /files/read.json endponit with
negative offset or length.
Diffs
-----
src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
Diff: https://reviews.apache.org/r/46373/diff/
Testing
-------
make
make check
request 'files/read.json' endpoint with negative offset or length argument
Thanks,
zhou xing
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review129748
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46373]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 20, 2016, 5:57 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 20, 2016, 5:57 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review130864
-----------------------------------------------------------
Fix it, then Ship it!
Looks great! Just a couple small comments below, and then I think it's ready for BenM to have a look.
src/files/files.cpp (line 463)
<https://reviews.apache.org/r/46373/#comment194774>
Now that `length` is a `size_t`, we can get rid of the `static_cast` here.
src/tests/files_tests.cpp (line 175)
<https://reviews.apache.org/r/46373/#comment194769>
End comment with a period.
src/tests/files_tests.cpp (line 183)
<https://reviews.apache.org/r/46373/#comment194770>
End comment with a period.
- Greg Mann
On April 26, 2016, 8:15 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 8:15 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review130883
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46373]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 28, 2016, 2:15 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 28, 2016, 2:15 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by zhou xing <xi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/
-----------------------------------------------------------
(Updated 四月 28, 2016, 2:15 a.m.)
Review request for mesos, Ben Mahler and Greg Mann.
Changes
-------
update the code according to Greg's comments
Bugs: mesos-5060
https://issues.apache.org/jira/browse/mesos-5060
Repository: mesos
Description
-------
[MESOS-5060]
The patch did the following changes:
1. Fix the length logic in files.cpp.
2. Add some tests to test the /files/read.json endponit with
negative length.
Diffs (updated)
-----
src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
Diff: https://reviews.apache.org/r/46373/diff/
Testing
-------
make
make check
request 'files/read.json' endpoint with negative offset or length argument
Thanks,
zhou xing
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review130575
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46373]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 26, 2016, 8:15 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 26, 2016, 8:15 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by zhou xing <xi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/
-----------------------------------------------------------
(Updated 四月 26, 2016, 8:15 a.m.)
Review request for mesos, Ben Mahler and Greg Mann.
Changes
-------
rebase and update code according to Greg's comments
Bugs: mesos-5060
https://issues.apache.org/jira/browse/mesos-5060
Repository: mesos
Description
-------
[MESOS-5060]
The patch did the following changes:
1. Fix the length logic in files.cpp.
2. Add some tests to test the /files/read.json endponit with
negative length.
Diffs (updated)
-----
src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
Diff: https://reviews.apache.org/r/46373/diff/
Testing
-------
make
make check
request 'files/read.json' endpoint with negative offset or length argument
Thanks,
zhou xing
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review130342
-----------------------------------------------------------
src/files/files.cpp (line 362)
<https://reviews.apache.org/r/46373/#comment194066>
I'm not sure that I like using -1 as a special number here. Perhaps we could use an `Option<size_t>` for `length`; see my comment below about this as well.
src/files/files.cpp (line 423)
<https://reviews.apache.org/r/46373/#comment194069>
If `length` was an `Option<size_t>`, then we could check for `length.isNone()` here.
src/tests/files_tests.cpp (line 175)
<https://reviews.apache.org/r/46373/#comment194068>
s/nagative/negative/
- Greg Mann
On April 20, 2016, 5:57 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 20, 2016, 5:57 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by zhou xing <xi...@cn.ibm.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/
-----------------------------------------------------------
(Updated 四月 20, 2016, 5:57 a.m.)
Review request for mesos, Ben Mahler and Greg Mann.
Changes
-------
rebased and updated the code according to Greg's comments
Bugs: mesos-5060
https://issues.apache.org/jira/browse/mesos-5060
Repository: mesos
Description (updated)
-------
[MESOS-5060]
The patch did the following changes:
1. Fix the length logic in files.cpp.
2. Add some tests to test the /files/read.json endponit with
negative length.
Diffs (updated)
-----
src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
Diff: https://reviews.apache.org/r/46373/diff/
Testing
-------
make
make check
request 'files/read.json' endpoint with negative offset or length argument
Thanks,
zhou xing
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review129528
-----------------------------------------------------------
Patch looks great!
Reviews applied: [46373]
Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On April 19, 2016, 8:07 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 8:07 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the offset and length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative offset or length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>
Re: Review Request 46373: Request /files/read.json with a negative
length value causes error.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46373/#review129620
-----------------------------------------------------------
src/files/files.cpp (line 380)
<https://reviews.apache.org/r/46373/#comment193055>
Rather than logging this and setting the length to page size here, I think we would want to return a BadRequest response.
src/files/files.cpp (line 426)
<https://reviews.apache.org/r/46373/#comment193056>
Negative offsets can be useful as well; for example, I could use {offset = -1024; length = 1024} to read the last 1024 bytes of the file.
- Greg Mann
On April 19, 2016, 8:07 a.m., zhou xing wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46373/
> -----------------------------------------------------------
>
> (Updated April 19, 2016, 8:07 a.m.)
>
>
> Review request for mesos, Ben Mahler and Greg Mann.
>
>
> Bugs: mesos-5060
> https://issues.apache.org/jira/browse/mesos-5060
>
>
> Repository: mesos
>
>
> Description
> -------
>
> [MESOS-5060]
> The patch did the following changes:
> 1. Fix the offset and length logic in files.cpp.
> 2. Add some tests to test the /files/read.json endponit with
> negative offset or length.
>
>
> Diffs
> -----
>
> src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1
> src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd
>
> Diff: https://reviews.apache.org/r/46373/diff/
>
>
> Testing
> -------
>
> make
> make check
>
> request 'files/read.json' endpoint with negative offset or length argument
>
>
> Thanks,
>
> zhou xing
>
>