Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9301026:
Virtual Me2Me Host: Exit process if Host ID is invalid. (Closed)
|
Created:
8 years, 10 months ago by Lambros Modified:
8 years, 10 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionVirtual Me2Me Host: Exit process if Host ID is invalid.
The exit-code from remoting_me2me_host will be used to communicate to the
daemon that the Host ID was rejected as invalid.
BUG=110046
TEST=Run remoting_me2me_host with invalid host.json
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120031
Patch Set 1 #
Total comments: 15
Patch Set 2 : Fix nits #
Total comments: 1
Messages
Total messages: 8 (0 generated)
some nits http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.cc File remoting/host/heartbeat_sender.cc (right): http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:16: #include "remoting/protocol/jingle_messages.h" don't need this include. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:32: const char kErrorTag[] = "error"; IMO it is not necessary to define it here if it is used in only one place. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:88: response->FirstNamed(QName(remoting::protocol::kJabberNamespace, You can use buzz::NS_CLIENT here instead to be consistent with NS_STANZA below. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:100: << response->Str(); revert this?
http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.cc File remoting/host/heartbeat_sender.cc (right): http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:32: const char kErrorTag[] = "error"; On 2012/01/31 02:37:13, sergeyu wrote: > IMO it is not necessary to define it here if it is used in only one place. 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. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:93: // shut down the host properly, instead of just exiting here. Create a Type-Cleanup bug for this and include the Id here? :) http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:95: exit(1); I think this should be _exit(1), so that destructors don't get called.
http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.cc File remoting/host/heartbeat_sender.cc (right): http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:32: const char kErrorTag[] = "error"; On 2012/01/31 05:44:03, Wez wrote: > On 2012/01/31 02:37:13, sergeyu wrote: > > IMO it is not necessary to define it here if it is used in only one place. > > We may as well define it, for consistency with the other tags. 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. > > nit: I think there are enough tags and attributes now for it to make sense to > list them alphabetically. 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. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:95: exit(1); On 2012/01/31 05:44:03, Wez wrote: > I think this should be _exit(1), so that destructors don't get called. 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.
http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.cc File remoting/host/heartbeat_sender.cc (right): http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:16: #include "remoting/protocol/jingle_messages.h" On 2012/01/31 02:37:13, sergeyu wrote: > don't need this include. Done. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:32: const char kErrorTag[] = "error"; 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: > On 2012/01/31 05:44:03, Wez wrote: > > On 2012/01/31 02:37:13, sergeyu wrote: > > > IMO it is not necessary to define it here if it is used in only one place. > > > > We may as well define it, for consistency with the other tags. > > 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. > > > > > nit: I think there are enough tags and attributes now for it to make sense to > > list them alphabetically. > > 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. > http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:88: response->FirstNamed(QName(remoting::protocol::kJabberNamespace, On 2012/01/31 02:37:13, sergeyu wrote: > You can use buzz::NS_CLIENT here instead to be consistent with NS_STANZA below. Done. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:93: // shut down the host properly, instead of just exiting here. On 2012/01/31 05:44:03, Wez wrote: > Create a Type-Cleanup bug for this and include the Id here? :) Done. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:95: exit(1); On 2012/01/31 07:41:35, sergeyu wrote: > On 2012/01/31 05:44:03, Wez wrote: > > I think this should be _exit(1), so that destructors don't get called. > > 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. 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. http://codereview.chromium.org/9301026/diff/1/remoting/host/heartbeat_sender.... remoting/host/heartbeat_sender.cc:100: << response->Str(); On 2012/01/31 02:37:13, sergeyu wrote: > revert this? Done.
lgtm http://codereview.chromium.org/9301026/diff/3001/remoting/host/heartbeat_send... File remoting/host/heartbeat_sender.cc (right): http://codereview.chromium.org/9301026/diff/3001/remoting/host/heartbeat_send... remoting/host/heartbeat_sender.cc:89: if (error_element->FirstNamed(QName(buzz::NS_STANZA, kNotFoundTag))) { nit: maybe roll these two if statements together, i.e. if (error_element && error_element->FirstNamed(...))
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/9301026/3001
Change committed as 120031 |
