Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-28 Thread Jie Yu 


> On Oct. 27, 2016, 5:03 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, lines 71-72
> > 
> >
> > I think James made a valid point there that errno might not be 
> > preserved across delete. So using a vector sounds better.
> 
> Qian Zhang wrote:
> I think `delete` will not change `errno`, I verified it by a small 
> program:
> ```
> #include 
> #include 
> 
> int main()
> {
>   char* temp = new char[4];
> 
>   if (open("/xxx", 0) < 0) {
> delete[] temp;
> return errno;
>   }
>  
>   delete[] temp;
>   return 0;
> }
> ```
> This program tries to open a file `/xxx` which does not exist, and when I 
> run it, its exit code is 2 (i.e., `ENOENT`). And actually in stout I see 
> there are also some other codes using `delete` and `ErrnoError` in the same 
> way, e.g.:
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/mktemp.hpp#L40:L41
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp#L37:L38
> 
> However, I do see there are also some codes which tries to avoid `delete` 
> affecting `ErrnoError` in this way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/read.hpp#L67:L69
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/sysctl.hpp#L200:L202
> 
> 
> So the codes in stout are inconsistent regarding this issue. I think we 
> should fix it by making them handle this issue in a consistent way, I would 
> prefer the following way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> Let me know your comments :-)
> 
> James Peach wrote:
> I thought about it and I think that Qian Zhan is right that `delete` 
> can't set `errno` because it never fails. If there was an error, it would 
> either throw or segv.

Ah, ok. Sounds good.


- Jie


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


On Oct. 25, 2016, 3:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 3:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-28 Thread James Peach 


> On Oct. 27, 2016, 5:03 p.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, lines 71-72
> > 
> >
> > I think James made a valid point there that errno might not be 
> > preserved across delete. So using a vector sounds better.
> 
> Qian Zhang wrote:
> I think `delete` will not change `errno`, I verified it by a small 
> program:
> ```
> #include 
> #include 
> 
> int main()
> {
>   char* temp = new char[4];
> 
>   if (open("/xxx", 0) < 0) {
> delete[] temp;
> return errno;
>   }
>  
>   delete[] temp;
>   return 0;
> }
> ```
> This program tries to open a file `/xxx` which does not exist, and when I 
> run it, its exit code is 2 (i.e., `ENOENT`). And actually in stout I see 
> there are also some other codes using `delete` and `ErrnoError` in the same 
> way, e.g.:
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/mktemp.hpp#L40:L41
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp#L37:L38
> 
> However, I do see there are also some codes which tries to avoid `delete` 
> affecting `ErrnoError` in this way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/read.hpp#L67:L69
> 
> https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/sysctl.hpp#L200:L202
> 
> 
> So the codes in stout are inconsistent regarding this issue. I think we 
> should fix it by making them handle this issue in a consistent way, I would 
> prefer the following way:
> ```
> Error error = ErrnoError();
> delete[] temp;
> return error;
> ```
> 
> Let me know your comments :-)

I thought about it and I think that Qian Zhan is right that `delete` can't set 
`errno` because it never fails. If there was an error, it would either throw or 
segv.


- James


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


On Oct. 25, 2016, 3:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 3:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-28 Thread Qian Zhang 


> On Oct. 28, 2016, 1:03 a.m., Jie Yu wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, lines 71-72
> > 
> >
> > I think James made a valid point there that errno might not be 
> > preserved across delete. So using a vector sounds better.

I think `delete` will not change `errno`, I verified it by a small program:
```
#include 
#include 

int main()
{
  char* temp = new char[4];

  if (open("/xxx", 0) < 0) {
delete[] temp;
return errno;
  }
 
  delete[] temp;
  return 0;
}
```
This program tries to open a file `/xxx` which does not exist, and when I run 
it, its exit code is 2 (i.e., `ENOENT`). And actually in stout I see there are 
also some other codes using `delete` and `ErrnoError` in the same way, e.g.:
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/mktemp.hpp#L40:L41
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/posix/mkdtemp.hpp#L37:L38

However, I do see there are also some codes which tries to avoid `delete` 
affecting `ErrnoError` in this way:
```
Error error = ErrnoError();
delete[] temp;
return error;
```
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/read.hpp#L67:L69
https://github.com/apache/mesos/blob/1.0.1/3rdparty/stout/include/stout/os/sysctl.hpp#L200:L202


So the codes in stout are inconsistent regarding this issue. I think we should 
fix it by making them handle this issue in a consistent way, I would prefer the 
following way:
```
Error error = ErrnoError();
delete[] temp;
return error;
```

Let me know your comments :-)


- Qian


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


On Oct. 25, 2016, 11:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 11:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-27 Thread Jie Yu 

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


Fix it, then Ship it!





3rdparty/stout/include/stout/os/posix/xattr.hpp (lines 71 - 72)


I think James made a valid point there that errno might not be preserved 
across delete. So using a vector sounds better.


- Jie Yu


On Oct. 25, 2016, 3:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 3:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-24 Thread James Peach 


> On Oct. 21, 2016, 4:19 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 26
> > 
> >
> > FreeBSD would need a third branch for the 
> > (extattr)[https://www.freebsd.org/cgi/man.cgi?query=extattr=2=0=FreeBSD+10.3-RELEASE+and+Ports]
> >  API. 
> > 
> > It looks like stout does attempt to support FreeBSD.
> 
> Qian Zhang wrote:
> Yes, FreeBSD also supports set/get extended attribute with a set of 
> different interfaces (e.g., `extattr_set_file`, `extattr_get_file`), but I 
> think Mesos does not support FreeBSD now, and such interface in FreeBSD is 
> not stable yet, see the following info I got from the manpage. So I would not 
> suggest to support FreeBSD for extended attribute in `stout` at this point.
> ```
> CAVEAT
>  This interface is under active development, and as such is subject to
>  change as applications are adapted to use it.  Developers are 
> discouraged
>  from relying on its stability.
> ```

Stout seems to support FreeBSD. If this API is not needed on the FreeBSD port 
right now, you should return ``ENOSYS`` for FreeBSD.


- James


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


On Oct. 25, 2016, 3:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 25, 2016, 3:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-24 Thread Qian Zhang 

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

(Updated Oct. 25, 2016, 11:41 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


Bugs: MESOS-6360
https://issues.apache.org/jira/browse/MESOS-6360


Repository: mesos


Description
---

Added `setxattr()` and `getxattr()` in stout.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
  3rdparty/stout/include/stout/os.hpp 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
  3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53041/diff/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-23 Thread James Peach 


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 24
> > 
> >
> > Since the flags differ between platform, it might be a good idea to 
> > define stout-specific versions.
> 
> Qian Zhang wrote:
> Can you elaborate on stout-specific versions? Did you mean we define two 
> versions of setxattr() here, one for Linux and another for APPLE?
> 
> James Peach wrote:
> I meant that the flags differ across the two platforms. So the caller has 
> to know what flags are safe to pass for both. For example, the API doesn't 
> prevent you trying to pass ``XATTR_NOFOLLOW`` on Linux, which won't build. 
> I'm wondering whether stout ought to define its own versions of the flags so 
> that you can more easily write portable code using this API.
> 
> Qian Zhang wrote:
> I'd like to expose the native OS flags to caller and let the caller to 
> decides what kind of flags should be used, e.g., if caller pass 
> `XATTR_NOFOLLOW` on Linux and find it can not built, then the caller should 
> put its code under `__APPLE__`.

It is a pretty poor abstraction if the caller has to ifdef. But since that is 
not needed now AFAICT, it doesn't matter much.


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 64
> > 
> >
> > You could use ``std::vector`` here to avoid manual memory 
> > management.
> 
> Qian Zhang wrote:
> Can you please let me know the advantage of std::vector? To me, new 
> + memset can satisfy my requirement here.
> 
> James Peach wrote:
> Dropped the issue since this is just a suggestion :)
> 
> IMHO, using vector is more robust since you get RAII behaviour and 
> don't need to explicitly delete on the error path, eg.
> 
> ```
> std::vector buffer(size);
> getxattr(path.c_str(), name.c_str(), [0], size, 0, 0);
> return std:string(buffer.begin(), buffer.size());
> 
> ```

Not sure that ``errno`` is guaranteed to be preserved across the ``delete``.


- James


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


On Oct. 23, 2016, 3:01 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 23, 2016, 3:01 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-22 Thread Qian Zhang 

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

(Updated Oct. 23, 2016, 11:01 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


Bugs: MESOS-6360
https://issues.apache.org/jira/browse/MESOS-6360


Repository: mesos


Description
---

Added `setxattr()` and `getxattr()` in stout.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
  3rdparty/stout/include/stout/os.hpp 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
  3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53041/diff/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-22 Thread Qian Zhang 


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 24
> > 
> >
> > Since the flags differ between platform, it might be a good idea to 
> > define stout-specific versions.
> 
> Qian Zhang wrote:
> Can you elaborate on stout-specific versions? Did you mean we define two 
> versions of setxattr() here, one for Linux and another for APPLE?
> 
> James Peach wrote:
> I meant that the flags differ across the two platforms. So the caller has 
> to know what flags are safe to pass for both. For example, the API doesn't 
> prevent you trying to pass ``XATTR_NOFOLLOW`` on Linux, which won't build. 
> I'm wondering whether stout ought to define its own versions of the flags so 
> that you can more easily write portable code using this API.

I'd like to expose the native OS flags to caller and let the caller to decides 
what kind of flags should be used, e.g., if caller pass `XATTR_NOFOLLOW` on 
Linux and find it can not built, then the caller should put its code under 
`__APPLE__`.


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 51
> > 
> >
> > Add a flags argument? Darwin's ``XATTR_NOFOLLOW`` can be mapped to 
> > ``lgetxattr`` on Linux.
> 
> Qian Zhang wrote:
> Did you mean if caller passes `XATTR_NOFOLLOW` as flag to this method on 
> Linux, internally we call `lgetxattr()`? But I think it may confuse the 
> caller since on Linux `getxattr()` does not support flags at all, so I would 
> suggest not to mix `getxattr()` and `lgetxattr()` in one method.
> 
> James Peach wrote:
> Yes, that makes sense. Later, you could add a ``lgetxattr`` wrapper.

Yeah, I agree, we can add it later.


- Qian


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


On Oct. 21, 2016, 5:29 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 21, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-22 Thread Qian Zhang 


> On Oct. 22, 2016, 12:19 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 26
> > 
> >
> > FreeBSD would need a third branch for the 
> > (extattr)[https://www.freebsd.org/cgi/man.cgi?query=extattr=2=0=FreeBSD+10.3-RELEASE+and+Ports]
> >  API. 
> > 
> > It looks like stout does attempt to support FreeBSD.

Yes, FreeBSD also supports set/get extended attribute with a set of different 
interfaces (e.g., `extattr_set_file`, `extattr_get_file`), but I think Mesos 
does not support FreeBSD now, and such interface in FreeBSD is not stable yet, 
see the following info I got from the manpage. So I would not suggest to 
support FreeBSD for extended attribute in `stout` at this point.
```
CAVEAT
 This interface is under active development, and as such is subject to
 change as applications are adapted to use it.  Developers are discouraged
 from relying on its stability.
```


- Qian


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


On Oct. 21, 2016, 5:29 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 21, 2016, 5:29 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-21 Thread James Peach 


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 64
> > 
> >
> > You could use ``std::vector`` here to avoid manual memory 
> > management.
> 
> Qian Zhang wrote:
> Can you please let me know the advantage of std::vector? To me, new 
> + memset can satisfy my requirement here.

Dropped the issue since this is just a suggestion :)

IMHO, using vector is more robust since you get RAII behaviour and don't 
need to explicitly delete on the error path, eg.

```
std::vector buffer(size);
getxattr(path.c_str(), name.c_str(), [0], size, 0, 0);
return std:string(buffer.begin(), buffer.size());

```


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 51
> > 
> >
> > Add a flags argument? Darwin's ``XATTR_NOFOLLOW`` can be mapped to 
> > ``lgetxattr`` on Linux.
> 
> Qian Zhang wrote:
> Did you mean if caller passes `XATTR_NOFOLLOW` as flag to this method on 
> Linux, internally we call `lgetxattr()`? But I think it may confuse the 
> caller since on Linux `getxattr()` does not support flags at all, so I would 
> suggest not to mix `getxattr()` and `lgetxattr()` in one method.

Yes, that makes sense. Later, you could add a ``lgetxattr`` wrapper.


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 24
> > 
> >
> > Since the flags differ between platform, it might be a good idea to 
> > define stout-specific versions.
> 
> Qian Zhang wrote:
> Can you elaborate on stout-specific versions? Did you mean we define two 
> versions of setxattr() here, one for Linux and another for APPLE?

I meant that the flags differ across the two platforms. So the caller has to 
know what flags are safe to pass for both. For example, the API doesn't prevent 
you trying to pass ``XATTR_NOFOLLOW`` on Linux, which won't build. I'm 
wondering whether stout ought to define its own versions of the flags so that 
you can more easily write portable code using this API.


- James


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


On Oct. 21, 2016, 9:29 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 21, 2016, 9:29 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-21 Thread James Peach 

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




3rdparty/stout/include/stout/os/posix/xattr.hpp (line 26)


FreeBSD would need a third branch for the 
(extattr)[https://www.freebsd.org/cgi/man.cgi?query=extattr=2=0=FreeBSD+10.3-RELEASE+and+Ports]
 API. 

It looks like stout does attempt to support FreeBSD.


- James Peach


On Oct. 21, 2016, 9:29 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 21, 2016, 9:29 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-21 Thread James Peach 


- James


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


On Oct. 21, 2016, 9:29 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 21, 2016, 9:29 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-21 Thread Qian Zhang 

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

(Updated Oct. 21, 2016, 5:29 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed James's comments.


Bugs: MESOS-6360
https://issues.apache.org/jira/browse/MESOS-6360


Repository: mesos


Description
---

Added `setxattr()` and `getxattr()` in stout.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
  3rdparty/stout/include/stout/os.hpp 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
  3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53041/diff/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-20 Thread Qian Zhang 


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 24
> > 
> >
> > Since the flags differ between platform, it might be a good idea to 
> > define stout-specific versions.

Can you elaborate on stout-specific versions? Did you mean we define two 
versions of setxattr() here, one for Linux and another for APPLE?


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 51
> > 
> >
> > Add a flags argument? Darwin's ``XATTR_NOFOLLOW`` can be mapped to 
> > ``lgetxattr`` on Linux.

Did you mean if caller passes `XATTR_NOFOLLOW` as flag to this method on Linux, 
internally we call `lgetxattr()`? But I think it may confuse the caller since 
on Linux `getxattr()` does not support flags at all, so I would suggest not to 
mix `getxattr()` and `lgetxattr()` in one method.


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 64
> > 
> >
> > You could use ``std::vector`` here to avoid manual memory 
> > management.

Can you please let me know the advantage of std::vector? To me, new + 
memset can satisfy my requirement here.


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 80
> > 
> >
> > Should you add ``removexattr`` for completeness?

Agree!


> On Oct. 20, 2016, 12:03 p.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/xattr.hpp, line 21
> > 
> >
> > I don't think this API makes sense for Windows. NTFS has streams, which 
> > have different semantics that extended attributes. I doubt that you would 
> > want this API on Windows.

Yes, I agree, let me mark those functions as delete on Windows.


- Qian


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


On Oct. 20, 2016, 11:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 20, 2016, 11:20 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-20 Thread Qian Zhang 

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




3rdparty/stout/include/stout/os/posix/xattr.hpp (line 24)


Can you elaborate on `stout-specific versions`? Did you mean we define two 
versions of `setxattr()` here, one for Linux and another for APPLE?



3rdparty/stout/include/stout/os/posix/xattr.hpp (line 51)


Did you mean if caller passes `XATTR_NOFOLLOW` as flag to this method on 
Linux, internally we call `lgetxattr()`? But I think it may confuse the caller 
since on Linux `getxattr()` does not support flags at all, so I would suggest 
not to mix `getxattr()` and `lgetxattr()` in one method.



3rdparty/stout/include/stout/os/posix/xattr.hpp (line 64)


Can you please let me know the advantage of `std::vector`? To me, 
`new` + `memset` can satisfy my requirement here.



3rdparty/stout/include/stout/os/posix/xattr.hpp (line 80)


Agree!



3rdparty/stout/include/stout/os/xattr.hpp (line 21)


Yes, I agree, let me mark those functions as delete on Windows.


- Qian Zhang


On Oct. 20, 2016, 11:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 20, 2016, 11:20 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-19 Thread James Peach 

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




3rdparty/stout/include/stout/os/posix/xattr.hpp (line 24)


Since the flags differ between platform, it might be a good idea to define 
stout-specific versions.



3rdparty/stout/include/stout/os/posix/xattr.hpp (line 51)


Add a flags argument? Darwin's ``XATTR_NOFOLLOW`` can be mapped to 
``lgetxattr`` on Linux.



3rdparty/stout/include/stout/os/posix/xattr.hpp (line 64)


You could use ``std::vector`` here to avoid manual memory management.



3rdparty/stout/include/stout/os/posix/xattr.hpp (line 80)


Should you add ``removexattr`` for completeness?



3rdparty/stout/include/stout/os/xattr.hpp (line 21)


I don't think this API makes sense for Windows. NTFS has streams, which 
have different semantics that extended attributes. I doubt that you would want 
this API on Windows.


- James Peach


On Oct. 20, 2016, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53041/
> ---
> 
> (Updated Oct. 20, 2016, 3:20 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `setxattr()` and `getxattr()` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
>   3rdparty/stout/include/stout/os.hpp 
> 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
>   3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53041/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 53041: Added `setxattr()` and `getxattr()` in stout.

2016-10-19 Thread Qian Zhang 

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

Review request for mesos.


Bugs: MESOS-6360
https://issues.apache.org/jira/browse/MESOS-6360


Repository: mesos


Description
---

Added `setxattr()` and `getxattr()` in stout.


Diffs
-

  3rdparty/stout/include/Makefile.am 1eb9c146d3eebacaea32e0f724d145f0c1dd676f 
  3rdparty/stout/include/stout/os.hpp 96e8621b198a3ec4cce78e0a6ff5f271eda05ff1 
  3rdparty/stout/include/stout/os/posix/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/xattr.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/xattr.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53041/diff/


Testing
---


Thanks,

Qian Zhang