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.
--- 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
Re: Review Request 26529: Modules should accept relative path or file name for library name.
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.
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.
---
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.
--- 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
