mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: Review Request 53041: Added `setxattr()` and `getxattr()` in stout.
Date Sun, 23 Oct 2016 20:40:12 GMT


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 24
> > <https://reviews.apache.org/r/53041/diff/1/?file=1541992#file1541992line24>
> >
> >     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
> > <https://reviews.apache.org/r/53041/diff/1/?file=1541992#file1541992line64>
> >
> >     You could use ``std::vector<char>`` here to avoid manual memory management.
> 
> Qian Zhang wrote:
>     Can you please let me know the advantage of std::vector<char>? To me, new +
memset can satisfy my requirement here.
> 
> James Peach wrote:
>     Dropped the issue since this is just a suggestion :)
>     
>     IMHO, using vector<char> is more robust since you get RAII behaviour and don't
need to explicitly delete on the error path, eg.
>     
>     ```
>     std::vector<char> buffer(size);
>     getxattr(path.c_str(), name.c_str(), &buffer[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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message