You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2014/10/10 17:24:35 UTC
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 11:24 a.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
filename->file; replaced macros with const char*.
Summary (updated)
-----------------
Modules should accept relative path or file name for library name.
Repository: mesos-git
Description (updated)
-------
Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
Diffs (updated)
-----
include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
Diff: https://reviews.apache.org/r/26529/diff/
Testing
-------
Added tests for LD_LIBRARY_PATH and ran make check.
Thanks,
Kapil Arya
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/#review56237
-----------------------------------------------------------
Ship it!
Ship It!
- Niklas Nielsen
On Oct. 10, 2014, 2:32 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26529/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 2:32 p.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
>
> Also, return a better error message if an empty module name is provided.
>
>
> Diffs
> -----
>
> include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
> src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
> src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
> src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
>
> Diff: https://reviews.apache.org/r/26529/diff/
>
>
> Testing
> -------
>
> Added tests for LD_LIBRARY_PATH and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 5:32 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Marked {old,new}LdPreload as const.
Repository: mesos-git
Description
-------
Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
Also, return a better error message if an empty module name is provided.
Diffs (updated)
-----
include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
Diff: https://reviews.apache.org/r/26529/diff/
Testing
-------
Added tests for LD_LIBRARY_PATH and ran make check.
Thanks,
Kapil Arya
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/
-----------------------------------------------------------
(Updated Oct. 10, 2014, 2:36 p.m.)
Review request for mesos and Niklas Nielsen.
Changes
-------
Used const string instead of string literal for LD_LIBRARY_PATH.
Repository: mesos-git
Description (updated)
-------
Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
Also, return a better error message if an empty module name is provided.
Diffs (updated)
-----
include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
Diff: https://reviews.apache.org/r/26529/diff/
Testing
-------
Added tests for LD_LIBRARY_PATH and ran make check.
Thanks,
Kapil Arya
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On Oct. 10, 2014, 8:31 a.m., Niklas Nielsen wrote:
> > src/module/manager.cpp, lines 191-193
> > <https://reviews.apache.org/r/26529/diff/2/?file=717123#file717123line191>
> >
> > Mind mentioning fixing this bug (and adding tests for it) in the RR description? We try to separate concerns / issues of RR's.
> > Also, do you have the library name handy so you can give a bit more detailed error description? Imagine a long list of modules where a single one didn't have the module name set :-)
>
> Kapil Arya wrote:
> Should I create a separate review request for the empty test(s)?
One or the other. The commit didn't include any details on the bug you fixed when renaming 'path' - I will leave that up to you.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/#review56147
-----------------------------------------------------------
On Oct. 10, 2014, 8:24 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26529/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 8:24 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
>
>
> Diffs
> -----
>
> include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
> src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
> src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
> src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
>
> Diff: https://reviews.apache.org/r/26529/diff/
>
>
> Testing
> -------
>
> Added tests for LD_LIBRARY_PATH and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 10, 2014, 11:31 a.m., Niklas Nielsen wrote:
> > src/module/manager.cpp, lines 191-193
> > <https://reviews.apache.org/r/26529/diff/2/?file=717123#file717123line191>
> >
> > Mind mentioning fixing this bug (and adding tests for it) in the RR description? We try to separate concerns / issues of RR's.
> > Also, do you have the library name handy so you can give a bit more detailed error description? Imagine a long list of modules where a single one didn't have the module name set :-)
Should I create a separate review request for the empty test(s)?
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/#review56147
-----------------------------------------------------------
On Oct. 10, 2014, 11:24 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26529/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 11:24 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
>
>
> Diffs
> -----
>
> include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
> src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
> src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
> src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
>
> Diff: https://reviews.apache.org/r/26529/diff/
>
>
> Testing
> -------
>
> Added tests for LD_LIBRARY_PATH and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 26529: Modules should accept relative path or file
name for library name.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26529/#review56147
-----------------------------------------------------------
src/module/manager.cpp
<https://reviews.apache.org/r/26529/#comment96471>
Mind mentioning fixing this bug (and adding tests for it) in the RR description? We try to separate concerns / issues of RR's.
Also, do you have the library name handy so you can give a bit more detailed error description? Imagine a long list of modules where a single one didn't have the module name set :-)
src/tests/module_tests.cpp
<https://reviews.apache.org/r/26529/#comment96470>
Let's try to avoid macro constants - can we do static const char* or define a helper that does this for you?
src/tests/module_tests.cpp
<https://reviews.apache.org/r/26529/#comment96472>
const strings if you don't intent to change them
How about hoisting the 'LD_LIBRARY_PATH' string and define it once as a constant?
- Niklas Nielsen
On Oct. 10, 2014, 8:24 a.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26529/
> -----------------------------------------------------------
>
> (Updated Oct. 10, 2014, 8:24 a.m.)
>
>
> Review request for mesos and Niklas Nielsen.
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Rename "path" in Modules protobuf to "file" since a "path" always referes to an absolute or a relative path, whereas "file" may refer to a path or a file name. If "file" contains a slash ("/"), it is considered an absolute or relative path, otherwise if is considered a file name. For file name, dlopen() automatically does a standard library path search to find the absolute path.
>
>
> Diffs
> -----
>
> include/mesos/module.hpp 733b1500ae3b833de4ce7585ba1b667ec164fdce
> src/messages/messages.proto edf1e4ef83dfdd89f494cd215e629a304a8b84c1
> src/module/manager.cpp 72041c06b28191dea244a39ba99666e53198decc
> src/tests/module_tests.cpp 6f9ee33f2f2d41dcc5bde9ef6326186f56e7c7fc
>
> Diff: https://reviews.apache.org/r/26529/diff/
>
>
> Testing
> -------
>
> Added tests for LD_LIBRARY_PATH and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>