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 Fri, 21 Oct 2016 16:19:39 GMT


> 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.

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());

```


> On Oct. 20, 2016, 4:03 a.m., James Peach wrote:
> > 3rdparty/stout/include/stout/os/posix/xattr.hpp, line 51
> > <https://reviews.apache.org/r/53041/diff/1/?file=1541992#file1541992line51>
> >
> >     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
> > <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?

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
> 
>


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