Host offline status reporting.

remoting/host/offline_status_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/offline_status_sender.h"
+
+#include "base/logging.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/stringize_macros.h"
+#include "base/time.h"
+#include "remoting/base/constants.h"
+#include "remoting/jingle_glue/iq_sender.h"
+#include "remoting/jingle_glue/signal_strategy.h"
+#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 kOfflineStatusTag[] = "offline-status";

[rmsousa, 2013/07/02 02:09:24]

Can you make the tag "host-status", and make "offline" a separate "status"
attribute?

+const char kHostIdAttr[] = "hostid";
+const char kHostVersionTag[] = "host-version";
+const char kHeartbeatSignatureTag[] = "signature";
+const char kStatusAttr[] = "status";

[rmsousa, 2013/07/02 02:09:24]

if we add the separate online/offline "status" attribute, this can be renamed
"exit-code".

+const char kErrorCodeAttr[] = "errorcode";
+
+}  // namespace
+
+OfflineStatusSender::OfflineStatusSender(

[rmsousa, 2013/07/02 02:09:24]

Please rename to HostStatusSender (see above)

+    const std::string& host_id,
+    SignalStrategy* signal_strategy,
+    scoped_refptr<RsaKeyPair> key_pair,
+    const std::string& directory_bot_jid)
+    : host_id_(host_id),
+      signal_strategy_(signal_strategy),
+      key_pair_(key_pair),
+      directory_bot_jid_(directory_bot_jid) {
+  DCHECK(signal_strategy_);
+  DCHECK(key_pair_.get());
+
+  VLOG(1) << "Creating OfflineStatusSender";
+
+  signal_strategy_->AddListener(this);
+}
+
+OfflineStatusSender::~OfflineStatusSender() {
+  signal_strategy_->RemoveListener(this);
+}
+
+void OfflineStatusSender::OnSignalStrategyStateChange(
+    SignalStrategy::State state) {
+  if (state == SignalStrategy::CONNECTED)
+    iq_sender_.reset(new IqSender(signal_strategy_));
+  else if (state == SignalStrategy::DISCONNECTED)
+    iq_sender_.reset();
+}
+
+bool OfflineStatusSender::OnSignalStrategyIncomingStanza(
+    const XmlElement* stanza) {
+  return false;
+}
+
+void OfflineStatusSender::SendOfflineStatus(int status, uint errorcode) {
+  SignalStrategy::State state = signal_strategy_->GetState();
+  if (state == SignalStrategy::CONNECTED) {
+    LOG(INFO) << "Sending offline status '"
+              << status
+              << "' to "
+              << directory_bot_jid_;
+
+    iq_sender_->SendIq(buzz::STR_SET,
+                       directory_bot_jid_,
+                       CreateOfflineStatusMessage(status, errorcode),
+                       IqSender::ReplyCallback());
+  } else {
+    LOG(INFO) << "Cannot send offline status to '"
+              << directory_bot_jid_
+              << " ' because the state of the SignalStrategy is "
+              << state;
+  }
+}
+
+scoped_ptr<XmlElement> OfflineStatusSender::CreateOfflineStatusMessage(
+    int status, uint errorcode) {
+  // Create offline status stanza.
+  scoped_ptr<XmlElement> offline_status(new XmlElement(
+      QName(kChromotingXmlNamespace, kOfflineStatusTag)));
+  offline_status->AddAttr(
+      QName(kChromotingXmlNamespace, kHostIdAttr), host_id_);
+  offline_status->AddAttr(
+      QName(kChromotingXmlNamespace, kStatusAttr), base::IntToString(status));
+  // The error code is currently unused and always contains a value of 0.

[rmsousa, 2013/07/02 02:09:24]

We should add this only when we know where it'll come from, and what we'll do
with it. Otherwise, it's complicated:

Without defining what will go in this field, we can't even know what type it
should be (Unix errnos are int, HRESULTs are longs, and GetLastErrors are
DWORDs).

And then, this stanza doesn't currently have the host's OS, so it's impossible
to know how to interpret this number from this stanza (i.e. is it an HRESULT, a
GetLastError() code, a UNIX errno?).

This might be something we will want to log in the future, but I'd prefer to
wait until we know the scenario before we design it. In general, adding
attributes to an existing structure is easy (be it an XML message, a protobuf, a
JSON structure, etc), but removing superfluous attributes is much harder (often
near-impossible, e.g. with protobufs)

+  // In the future we may use it for things like windows error code.

[rmsousa, 2013/07/02 02:09:24]

Nit: Comments describing the arguments should be in the header.

+  offline_status->AddAttr(
+      QName(kChromotingXmlNamespace, kErrorCodeAttr),
+      base::IntToString(errorcode));
+  offline_status->AddElement(CreateSignature().release());
+  // Append host version.
+  scoped_ptr<XmlElement> version_tag(new XmlElement(
+      QName(kChromotingXmlNamespace, kHostVersionTag)));
+  version_tag->AddText(STRINGIZE(VERSION));
+  offline_status->AddElement(version_tag.release());

[rmsousa, 2013/07/02 02:09:24]

please piggyback a log message, like heartbeat sender does. 

+  return offline_status.Pass();
+}
+
+scoped_ptr<XmlElement> OfflineStatusSender::CreateSignature() {
+  scoped_ptr<XmlElement> signature_tag(new XmlElement(
+      QName(kChromotingXmlNamespace, kHeartbeatSignatureTag)));

[rmsousa, 2013/07/02 02:09:24]

s/Heartbeat/OfflineStatus

+
+  std::string message = signal_strategy_->GetLocalJid();

[rmsousa, 2013/07/02 02:09:24]

I'm not sure we need to sign this message, but if we do, we should sign the
actual payload (status) along with the JID.

+  std::string signature(key_pair_->SignMessage(message));
+  signature_tag->AddText(signature);
+
+  return signature_tag.Pass();
+}
+
+}  // namespace remoting