Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Jojy Varghese
Yes thats correct. I am planning to submit a patch that will add the test. Will 
also address the symlink case.

-jojy

> On Mar 15, 2016, at 3:33 PM, Cong Wang  wrote:
> 
> This is an automatically generated e-mail. To reply, 
> visit:https://reviews.apache.org/r/44230/ 
> 
> On March 15th, 2016, 8:02 p.m. UTC, Neil Conway wrote:
> 
> Is it feasible/portable to have a test case for this change?
> On March 15th, 2016, 8:58 p.m. UTC, Cong Wang wrote:
> 
> Yes, like in our case, you can create some socket or device file and try to 
> remove the directory contains it, it would fail without this patch.
> On March 15th, 2016, 9:26 p.m. UTC, David Robinson wrote:
> 
> AFAICT tests have already been added:
> https://github.com/apache/mesos/commit/c55ca2941b0119c13764bccdebcea46119e69e4e
> None of these tests covers this bug.
> 
> - Cong
> 
> 
> On March 1st, 2016, 10:01 p.m. UTC, Jojy Varghese wrote:
> 
> Review request for mesos and Jie Yu.
> By Jojy Varghese.
> Updated March 1, 2016, 10:01 p.m.
> 
> Repository: mesos
> Description
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> Testing
> 
> make check.
> Diffs
> 
> 3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> (bc420c9c10d93ddd619a9eb2c5f4db67f31d722f)
> View Diff 


Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Cong Wang


> On March 15, 2016, 8:02 p.m., Neil Conway wrote:
> > Is it feasible/portable to have a test case for this change?
> 
> Cong Wang wrote:
> Yes, like in our case, you can create some socket or device file and try 
> to remove the directory contains it, it would fail without this patch.
> 
> David Robinson wrote:
> AFAICT tests have already been added:
> 
> 
> https://github.com/apache/mesos/commit/c55ca2941b0119c13764bccdebcea46119e69e4e

None of these tests covers this bug.


- Cong


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


On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/
> ---
> 
> (Updated March 1, 2016, 10:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/44230/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Jojy Varghese
Would be happy to add test. Patch forthcoming. Will also address FTS_SLNONE.

-Jojy


> On Mar 15, 2016, at 2:26 PM, David Robinson  
> wrote:
> 
> 
> 
>> On March 15, 2016, 8:02 p.m., Neil Conway wrote:
>>> Is it feasible/portable to have a test case for this change?
>> 
>> Cong Wang wrote:
>>Yes, like in our case, you can create some socket or device file and try 
>> to remove the directory contains it, it would fail without this patch.
> 
> AFAICT tests have already been added:
> 
> https://github.com/apache/mesos/commit/c55ca2941b0119c13764bccdebcea46119e69e4e
> 
> 
> - David
> 
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/#review123733
> ---
> 
> 
> On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
>> 
>> ---
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/44230/
>> ---
>> 
>> (Updated March 1, 2016, 10:01 p.m.)
>> 
>> 
>> Review request for mesos and Jie Yu.
>> 
>> 
>> Repository: mesos
>> 
>> 
>> Description
>> ---
>> 
>> We currently dont handle special files like device files in rmdir. This 
>> change
>> adds FS_DEFAULT as one of the cases where we try to unlink a file.
>> 
>> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
>> 
>> 
>> Diffs
>> -
>> 
>>  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
>> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
>> 
>> Diff: https://reviews.apache.org/r/44230/diff/
>> 
>> 
>> Testing
>> ---
>> 
>> make check.
>> 
>> 
>> Thanks,
>> 
>> Jojy Varghese
>> 
>> 
> 



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread David Robinson


> On March 15, 2016, 8:02 p.m., Neil Conway wrote:
> > Is it feasible/portable to have a test case for this change?
> 
> Cong Wang wrote:
> Yes, like in our case, you can create some socket or device file and try 
> to remove the directory contains it, it would fail without this patch.

AFAICT tests have already been added:

https://github.com/apache/mesos/commit/c55ca2941b0119c13764bccdebcea46119e69e4e


- David


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


On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/
> ---
> 
> (Updated March 1, 2016, 10:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/44230/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Cong Wang


> On March 15, 2016, 8:02 p.m., Neil Conway wrote:
> > Is it feasible/portable to have a test case for this change?

Yes, like in our case, you can create some socket or device file and try to 
remove the directory contains it, it would fail without this patch.


- Cong


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


On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/
> ---
> 
> (Updated March 1, 2016, 10:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/44230/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Neil Conway

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



Also, seems pretty clear we should handle `FTS_SLNONE`, as the TODO suggests.

- Neil Conway


On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/
> ---
> 
> (Updated March 1, 2016, 10:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/44230/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-15 Thread Neil Conway

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



Is it feasible/portable to have a test case for this change?

- Neil Conway


On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/
> ---
> 
> (Updated March 1, 2016, 10:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/44230/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>



Re: Review Request 44230: Added FS_DEFAULT case in rmdir.

2016-03-01 Thread Jie Yu

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


Ship it!




Ship It!

- Jie Yu


On March 1, 2016, 10:01 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44230/
> ---
> 
> (Updated March 1, 2016, 10:01 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We currently dont handle special files like device files in rmdir. This change
> adds FS_DEFAULT as one of the cases where we try to unlink a file.
> 
> Reference: http://man7.org/linux/man-pages/man3/fts.3.html
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
> bc420c9c10d93ddd619a9eb2c5f4db67f31d722f 
> 
> Diff: https://reviews.apache.org/r/44230/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>