Improved yarp run apis and tests.#3334
Conversation
Run.cpp file divided into seprate files: server and client. Improved `ps` funzionality to provide list of all running processes
|
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update the release notes by adding a file in |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the yarp::run library by splitting the monolithic Run.cpp into separate RunClient.cpp and RunServer.cpp files. The public API has been updated to return bool instead of int for many operations, and several internal methods now correctly use const references for Bottle parameters. Additionally, the test suite was expanded to cover more process management scenarios. Feedback focuses on a potential data race with a shared random number generator, a risk of segmentation faults due to insufficient validation of network responses, and various code quality improvements including the removal of unused variables and redundant logic.
| static std::random_device rd; | ||
| static std::mt19937 gen(rd()); | ||
| static std::uniform_int_distribution<> dist(0, 99999); |
There was a problem hiding this comment.
The static random number generator gen is shared across all threads calling sendMsg. Since std::mt19937 is not thread-safe, concurrent access from multiple threads can lead to data races and undefined behavior. Consider using a thread-local generator or protecting access with a mutex.
| yarp::os::Bottle* b = response.get(i).asList(); | ||
|
|
||
| temp.pid = b->get(0).asList()->get(1).asInt32(); | ||
| temp.tag = b->get(1).asList()->get(1).asString(); | ||
| temp.status = b->get(2).asList()->get(1).asString(); | ||
| temp.command = b->get(3).asList()->get(1).asString(); | ||
| temp.env = b->get(4).asList()->get(1).asString(); |
There was a problem hiding this comment.
This code is highly fragile and prone to crashes. It assumes that response.get(i) is a list and that its elements are also lists with specific indices. If the server returns an unexpected format, asList() will return nullptr, leading to a null pointer dereference and a segmentation fault. You should validate the structure of the Bottle before accessing its elements.
|
|
||
| bool connected = yarp::os::Network::connect(port.getName(), config.find("on").asString()); | ||
| std::string tempportname = port.getName(); | ||
| bool bb = yarp::os::Network::isConnected(tempportname, target); |
There was a problem hiding this comment.
The variable bb is initialized but never used. It should be removed to avoid compiler warnings and improve code clarity.
| sprintf(buff, "%d", response.get(0).asInt32()); | ||
| keyv=std::string(buff); | ||
|
|
||
| return response.get(0).asInt32()>0?true:false; |
There was a problem hiding this comment.
The ternary operator ? true : false is redundant here as the condition response.get(0).asInt32() > 0 already evaluates to a boolean value. This pattern is repeated in several other methods in this file (e.g., lines 535, 555, 577).
return response.get(0).asInt32() > 0;| response=sendMsg(msg, node); | ||
| for (size_t i=0; i<response.size(); i++) | ||
| { | ||
| response.get(i).toString(); |
There was a problem hiding this comment.
This statement has no effect and should be removed.
| #if defined(_WIN32) | ||
| int yarp::run::Run::server() | ||
| { | ||
| //yarp::os::Semaphore serializer(1); ///?????unused |
There was a problem hiding this comment.
Commented-out code and "unused" markers should be removed to maintain a clean codebase.
1 participant
Run.cpp file divided into seprate files: server and client.
Improved
psfunzionality to provide list of all running processes