790428 - C++ sutagent needs heartbeat channel

Status: RESOLVED FIXED

(Testing Graveyard :: SUTAgent, defect)

Comment 1

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 2

[Mark Côté [:mcote]]

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

Comment 3

[Mihnea Dobrescu-Balaur (:mihneadb)]

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

Comment 4

[Mark Côté [:mcote]]

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

Comment 5

[Mihnea Dobrescu-Balaur (:mihneadb)]

Thanks, long live valgrind ;)

Comment 6

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Rebased

Comment 7

[Mihnea Dobrescu-Balaur (:mihneadb)]

Attachment: Move cmdeventhandler functions back

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

Comment 8

[Mark Côté [:mcote]]

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.

Comment 9

[Mihnea Dobrescu-Balaur (:mihneadb)]

https://github.com/mihneadb/Negatus/commit/51f997c9fdfd1e28a79d0ef232780966a49767f6