C++ sutagent needs heartbeat channel

RESOLVED FIXED

Status

Testing Graveyard
SUTAgent
RESOLVED FIXED
6 years ago
6 months ago

People

(Reporter: mihneadb, Assigned: mihneadb)

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

6 years ago
Assignee: nobody → mbalaur
(Assignee)

Comment 1

6 years ago
Created attachment 660641 [details] [diff] [review]
Restructure and implement the functionality as discussed

https://github.com/mihneadb/Negatus/tree/factory

I had to rewrite those loops in Reactor.cpp because they would segfault. Removing the first / last element was buggy. It's safer now.
Attachment #660641 - Flags: feedback?(mcote)

Comment 2

6 years ago
Looks like you didn't pull from me. I already have fixes to those loops along with asynchronizing the exec command. Sorry but can you rebase your patch? We'd gotten into a back-and-forth pattern lately so I didn't think to send a pull request or anything. It shouldn't conflict much except the Reactor loop fixes (would prefer if you kept mine).
(Assignee)

Comment 3

6 years ago
Sure, I'll update and repost the diff. :)

Comment 4

6 years ago
Good catch regardless - those Reactor bugs took me a while to sort out the other day. :)
(Assignee)

Comment 5

6 years ago
Thanks, long live valgrind ;)
(Assignee)

Comment 6

6 years ago
Created attachment 660704 [details] [diff] [review]
Rebased
Attachment #660641 - Attachment is obsolete: true
Attachment #660641 - Flags: feedback?(mcote)
Attachment #660704 - Flags: feedback?(mcote)
(Assignee)

Comment 7

6 years ago
Created attachment 661010 [details] [diff] [review]
Move cmdeventhandler functions back

https://github.com/mihneadb/Negatus/tree/factory
Attachment #660704 - Attachment is obsolete: true
Attachment #660704 - Flags: feedback?(mcote)
Attachment #661010 - Flags: review?(mcote)

Comment 8

6 years ago
Comment on attachment 661010 [details] [diff] [review]
Move cmdeventhandler functions back

Review of attachment 661010 [details] [diff] [review]:
-----------------------------------------------------------------

This is great. :) Just a few comments below. Also if you wanted to merge the factory files into the event handler files (e.g. EventHandlerFactory.h/.cpp into EventHandler.h/.cpp, CommandEventHandlerFactory.h/.cpp into CommandEventHandler.h/.cpp, etc.), I would be okay with that. Up to you. :)

r+ with the small changes below.

::: src/CommandEventHandlerFactory.cpp
@@ +9,5 @@
> +
> +EventHandler*
> +CommandEventHandlerFactory::createEventHandler(PRFileDesc* socket)
> +{
> +    return new CommandEventHandler(socket);

2-space indents.

::: src/CommandEventHandlerFactory.h
@@ +11,5 @@
> +
> +class CommandEventHandlerFactory: public EventHandlerFactory
> +{
> +public:
> +    virtual EventHandler* createEventHandler(PRFileDesc* socket);

2-space idents.

::: src/EventHandlerFactory.h
@@ +9,5 @@
> +
> +
> +class EventHandlerFactory {
> +public:
> +    virtual EventHandler* createEventHandler(PRFileDesc* socket) = 0;

Ditto.

::: src/HeartbeatEventHandler.cpp
@@ +86,5 @@
> +void
> +HeartbeatEventHandler::handleTimeout()
> +{
> +  sendThump();
> +  setTimeout(PR_SecondsToInterval(TIMEOUT));

It might be better to set the timeout before sending the thump to get this closer to once every minute. I might have to put in a recurring timeout function to really prevent it from wandering, though.

@@ +96,5 @@
> +  PRTime now = PR_Now();
> +  PRExplodedTime ts;
> +  PR_ExplodeTime(now, PR_LocalTimeParameters, &ts);
> +
> +  char buffer[BUFSIZE];

You know how big this string is going to be, so I don't think there's any point in allocating a huge buffer.

::: src/SUTAgent.cpp
@@ +100,5 @@
> +  PRStatus status = acceptor->listen(acceptorAddr);
> +  if (status == PR_FAILURE)
> +  {
> +    std::cerr << "Failure to open socket: " << PR_GetError() << std::endl;
> +    exit(1);

I find exit calls in random functions to be very confusing; I'd prefer this function return a bool and have main() return if there's an error.

::: src/Shell.cpp
@@ +21,5 @@
> +    if (!iface)
> +      continue;
> +
> +    fgets(buffer, BUFSIZE, iface);
> +    buffer[strlen(buffer) - 1] = '\0'; // remove extra newline

I get a compilation error here; you need to #include <string.h>.

::: src/SocketAcceptor.h
@@ +23,4 @@
>  
>  private:
>    PRFileDesc* mSocket;
> +  EventHandlerFactory* handlerFactory;

Call this mHandlerFactory for consistency.
Attachment #661010 - Flags: review?(mcote) → review+
(Assignee)

Comment 9

6 years ago
https://github.com/mihneadb/Negatus/commit/51f997c9fdfd1e28a79d0ef232780966a49767f6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 months ago
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.