Include version information in heartbeats.

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 <math.h>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
 #include "base/rand_util.h"
 #include "base/string_number_conversions.h"
+#include "base/strings/stringize_macros.h"
 #include "base/time.h"
 #include "remoting/base/constants.h"
 #include "remoting/host/server_log_entry.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 kHeartbeatQueryTag[] = "heartbeat";
 const char kHostIdAttr[] = "hostid";
+const char kHostVersionAttr[] = "host-version";
 const char kHeartbeatSignatureTag[] = "signature";
 const char kSequenceIdAttr[] = "sequence-id";
 
 const char kErrorTag[] = "error";
 const char kNotFoundTag[] = "item-not-found";
 
 const char kHeartbeatResultTag[] = "heartbeat-result";
 const char kSetIntervalTag[] = "set-interval";
 const char kExpectedSequenceIdTag[] = "expected-sequence-id";
 
@@ skipping 193 matching lines @@
   sequence_id_was_set_ = true;
 }
 
 scoped_ptr<XmlElement> HeartbeatSender::CreateHeartbeatMessage() {
   // Create heartbeat stanza.
   scoped_ptr<XmlElement> query(new XmlElement(
       QName(kChromotingXmlNamespace, kHeartbeatQueryTag)));
   query->AddAttr(QName(kChromotingXmlNamespace, kHostIdAttr), host_id_);
   query->AddAttr(QName(kChromotingXmlNamespace, kSequenceIdAttr),
                  base::IntToString(sequence_id_));
+  query->AddAttr(QName(kChromotingXmlNamespace, kHostVersionAttr),

[Sergey Ulanov, 2013/02/11 20:29:11]

nit: Does this need to be an attribute? It may be better add it as a separate
element, so the heartbeat looks as follows:
 <heartbeat hostid=".." sequence-id="123">
   <signature>..Signature..</signature>
   <host-version>..version..</host-version>
 </heartbeat>

[Jamie, 2013/02/11 20:41:25]

Either works for me. I'd be interested to know what the pros and cons are of
your proposed change though.

[Sergey Ulanov, 2013/02/11 21:40:49]

There are no hard rules about it, but in general elements should be preferred,
particularly because they are easier to extend. In this case putting
host-version in an element would be better because in the future we may want to
add new fields, e.g. we may need to include OS version.

Here is a good article on the topic of attributes vs elements in XML:
http://www.ibm.com/developerworks/xml/library/x-eleatt/index.html
E.g. it says
"- If the information in question could be itself marked up with elements, put
it in an element."

[Jamie, 2013/02/13 18:25:26]

Thanks for the summary. Done.

+                 STRINGIZE(VERSION));
   query->AddElement(CreateSignature().release());
   // Append log message (which isn't signed).
   scoped_ptr<XmlElement> log(ServerLogEntry::MakeStanza());

[Sergey Ulanov, 2013/02/11 20:29:11]

The log entry should already include information about the host version. Can we
just reuse it instead of duplicating it in the same message?

[Jamie, 2013/02/11 20:41:25]

Renato suggested that in the bug discussion, but described it as a hack. You
guys know the server side a lot better than I do, so I'll defer to your
judgement. One advantage of that approach is that this CL isn't needed and we
have the information we need for older hosts.

Renato, do you agree with Sergey's preference to reuse the information from the
log stanza?

[Sergey Ulanov, 2013/02/11 21:40:49]

Yes. I agree. Given that we want to store this information in the directory,
reusing logs would be a hack.

[Jamie, 2013/02/13 18:25:26]

So it sounds like you're in favour of the approach taken in this CL, correct?

[Sergey Ulanov, 2013/02/13 18:53:15]

Yes, I'm in favor of this approach. Sorry for not being clear.

   scoped_ptr<ServerLogEntry> log_entry(ServerLogEntry::MakeForHeartbeat());
   log_entry->AddHostFields();
   log->AddElement(log_entry->ToStanza().release());
   query->AddElement(log.release());
   return query.Pass();
 }
 
 scoped_ptr<XmlElement> HeartbeatSender::CreateSignature() {
   scoped_ptr<XmlElement> signature_tag(new XmlElement(
       QName(kChromotingXmlNamespace, kHeartbeatSignatureTag)));
 
   std::string message = signal_strategy_->GetLocalJid() + ' ' +
       base::IntToString(sequence_id_);
   std::string signature(key_pair_->GetSignature(message));
   signature_tag->AddText(signature);
 
   return signature_tag.Pass();
 }
 
 }  // namespace remoting