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

remoting/host/heartbeat_sender.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/host/heartbeat_sender.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
 #include "base/string_number_conversions.h"
 #include "base/time.h"
 #include "remoting/base/constants.h"
 #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"
 
 using buzz::QName;
 using buzz::XmlElement;
 
 namespace remoting {
 
 namespace {
 
 const char kHeartbeatQueryTag[] = "heartbeat";
 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";
 
 const int64 kDefaultHeartbeatIntervalMs = 5 * 60 * 1000;  // 5 minutes.
 
 }  // namespace
 
 HeartbeatSender::HeartbeatSender(
     const std::string& host_id,
     SignalStrategy* signal_strategy,
@@ skipping 32 matching lines @@
   VLOG(1) << "Sending heartbeat stanza to " << kChromotingBotJid;
   request_.reset(iq_sender_->SendIq(
       buzz::STR_SET, kChromotingBotJid, CreateHeartbeatMessage(),
       base::Bind(&HeartbeatSender::ProcessResponse,
                  base::Unretained(this))));
 }
 
 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;
   }
 
   // This method must only be called for error or result stanzas.
   DCHECK_EQ(std::string(buzz::STR_RESULT), type);
 
   const XmlElement* result_element =
       response->FirstNamed(QName(kChromotingXmlNamespace, kHeartbeatResultTag));
   if (result_element) {
     const XmlElement* set_interval_element =
@@ skipping 43 matching lines @@
       QName(kChromotingXmlNamespace, kSignatureTimeAttr), time_str);
 
   std::string message = signal_strategy_->GetLocalJid() + ' ' + time_str;
   std::string signature(key_pair_->GetSignature(message));
   signature_tag->AddText(signature);
 
   return signature_tag;
 }
 
 }  // namespace remoting