Virtual Me2Me Host: Exit process if Host ID is invalid.

remoting/host/heartbeat_sender.cc

Index: remoting/host/heartbeat_sender.cc
diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index f1ba5151360a22e2c3e750518cc2cec7ae65147a..59d03d7093d1247899e98086f9819baa3498e117 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -13,6 +13,7 @@
 #include "remoting/jingle_glue/iq_sender.h"
 #include "remoting/jingle_glue/jingle_thread.h"
 #include "remoting/jingle_glue/signal_strategy.h"
+#include "remoting/protocol/jingle_messages.h"

[Sergey Ulanov, 2012/01/31 02:37:13]

don't need this include.

[Lambros, 2012/01/31 21:57:29]

Done.

 #include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
 #include "third_party/libjingle/source/talk/xmpp/constants.h"
 
@@ -28,6 +29,9 @@ const char kHostIdAttr[] = "hostid";
 const char kHeartbeatSignatureTag[] = "signature";
 const char kSignatureTimeAttr[] = "time";
 
+const char kErrorTag[] = "error";

[Sergey Ulanov, 2012/01/31 02:37:13]

IMO it is not necessary to define it here if it is used in only one place.

[Wez, 2012/01/31 05:44:03]

We may as well define it, for consistency with the other tags.

nit: I think there are enough tags and attributes now for it to make sense to
list them alphabetically.

[Sergey Ulanov, 2012/01/31 07:41:35]

Or alternatively we could remove other constants too :-P
I didn't define many constants in jingle_messages.cc and IMHO it makes code more
readable. The only minor downside of not defining them is the higher chance of
typos in case when the same value are repeated in several places (which should
be caught by unittests ;))
In this file consts are used in only one place each, so I don't think it makes
sense to define them here.
If we do define them then IMHO it is better to group them logically (e.g.
set-interval next to heartbeat-result) instead of alphabetically, as it makes it
easier to understand which of them are used for what.

[Lambros, 2012/01/31 21:57:29]

Normally, I'd agree that defining a constant for something only used once is
silly.  In this case, we're already using enough constants (many from other
files, like buzz::QN_TYPE) that it makes sense to keep them all as constants,
just for consistency.

On 2012/01/31 07:41:35, sergeyu wrote:

+const char kNotFoundTag[] = "item-not-found";
+
 const char kHeartbeatResultTag[] = "heartbeat-result";
 const char kSetIntervalTag[] = "set-interval";
 
@@ -80,8 +84,20 @@ void HeartbeatSender::DoSendStanza() {
 void HeartbeatSender::ProcessResponse(const XmlElement* response) {
   std::string type = response->Attr(buzz::QN_TYPE);
   if (type == buzz::STR_ERROR) {
+    const XmlElement* error_element =
+        response->FirstNamed(QName(remoting::protocol::kJabberNamespace,

[Sergey Ulanov, 2012/01/31 02:37:13]

You can use buzz::NS_CLIENT here instead to be consistent with NS_STANZA below.

[Lambros, 2012/01/31 21:57:29]

Done.

+                                   kErrorTag));
+    if (error_element) {
+      if (error_element->FirstNamed(QName(buzz::NS_STANZA, kNotFoundTag))) {
+        // TODO(lambroslambrou): Trigger an application-defined callback to
+        // shut down the host properly, instead of just exiting here.

[Wez, 2012/01/31 05:44:03]

Create a Type-Cleanup bug for this and include the Id here? :)

[Lambros, 2012/01/31 21:57:29]

Done.

+        LOG(ERROR) << "Received error: Host ID invalid";
+        exit(1);

[Wez, 2012/01/31 05:44:03]

I think this should be _exit(1), so that destructors don't get called.

[Sergey Ulanov, 2012/01/31 07:41:35]

As I understand the only difference is that _exit() doesn't call static
finalizers and atexit() handlers, but we shouldn't have any static finalizers or
atexit() handlers anyway, so I don't think this matters much.

[Lambros, 2012/01/31 21:57:29]

I think exit() is fine here.  It will run atexit()/on_exit() handlers (including
global dtors) and it will clean up tmpfiles() and flush stdio buffers.  But it
doesn't do stack-unwinding AFAIK
(http://stackoverflow.com/questions/7054685/are-destructors-run-when-calling-exit).
 The _exit() function is not cross-platform, and I think is intended for
terminating a forked process on Unix.

+      }
+    }
+
     LOG(ERROR) << "Received error in response to heartbeat: "
-               << response->Str();
+                 << response->Str();

[Sergey Ulanov, 2012/01/31 02:37:13]

revert this?

[Lambros, 2012/01/31 21:57:29]

Done.

     return;
   }