Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1821393002:
Adding the message definitions for the remote_security_key STDIO communication classes. (Closed)
|
Created:
4 years, 9 months ago by joedow Modified:
4 years, 8 months ago Reviewers:
Sergey Ulanov CC:
chromium-reviews, chromoting-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding the message definitions for the remote_security_key STDIO communication classes.
This is the first of several CLs which will introduce the classes needed
by the remote_security_key_process to communicate with security key enabled
tools.
BUG=584463
Committed: https://crrev.com/fd9d9f4e926a8471e2bdeb621e0916e4edba513c
Cr-Commit-Position: refs/heads/master@{#384128}
Patch Set 1 #Patch Set 2 : Cleanup berfore review #
Total comments: 10
Patch Set 3 : Addressing feedback #
Total comments: 19
Patch Set 4 : Addressing feedback #Patch Set 5 : Adding some enum value checking code #Patch Set 6 : Fixing a non-windows build break #
Total comments: 2
Patch Set 7 : Updating enum translation function per feedback #
Messages
Total messages: 18 (4 generated)
joedow@chromium.org changed reviewers: + sergeyu@chromium.org
This is the first CL in a series of CLs which implement that last pieces of the remote security key feature. PTAL! Thanks, Joe
https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_messages.cc (right): https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.cc:8: const uint32_t remoting::kMaxRemoteSecurityKeyMessageByteCount = 256 * 1024; I think all these constants can be moved to the .h file. They don't need this .cc file. https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.cc:18: const uint8_t remoting::kConnectToSecurityKeyEnabledSessionMsg = 0; I think these can be replaced with an enum. https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.cc:23: const uint8_t remoting::kRemoteSecurityKeyRequestMsg = 3; And these https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_messages.h (right): https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.h:25: // Message Format Are you going to put formatting/parsing code in the same file? If you put both of them in the same .cc file then all of these constants would not need to be defined in a header. The format looks very simple, so it doesn't seem necessary to separate parsing and formatting. E.g. we could have SecurityKeyMessage struct with two static method to format/parse the message. Similar to remoting/protocol/jingle_messages.h. WDYT? https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.h:30: // Header: Defines the length of the message (Control Code + Payload). Specify byte order please
Addressed CR feedback, PTAL! https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_messages.cc (right): https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.cc:8: const uint32_t remoting::kMaxRemoteSecurityKeyMessageByteCount = 256 * 1024; On 2016/03/23 19:07:25, Sergey Ulanov wrote: > I think all these constants can be moved to the .h file. They don't need this > .cc file. Done. https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.cc:18: const uint8_t remoting::kConnectToSecurityKeyEnabledSessionMsg = 0; On 2016/03/23 19:07:25, Sergey Ulanov wrote: > I think these can be replaced with an enum. Done. https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.cc:23: const uint8_t remoting::kRemoteSecurityKeyRequestMsg = 3; On 2016/03/23 19:07:25, Sergey Ulanov wrote: > And these Done. https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... File remoting/host/security_key/remote_security_key_messages.h (right): https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.h:25: // Message Format On 2016/03/23 19:07:25, Sergey Ulanov wrote: > Are you going to put formatting/parsing code in the same file? If you put both > of them in the same .cc file then all of these constants would not need to be > defined in a header. The format looks very simple, so it doesn't seem necessary > to separate parsing and formatting. > E.g. we could have SecurityKeyMessage struct with two static method to > format/parse the message. Similar to remoting/protocol/jingle_messages.h. WDYT? Done. https://codereview.chromium.org/1821393002/diff/20001/remoting/host/security_... remoting/host/security_key/remote_security_key_messages.h:30: // Header: Defines the length of the message (Control Code + Payload). On 2016/03/23 19:07:25, Sergey Ulanov wrote: > Specify byte order please Done.
I'll start looking at the reader and writer CLs https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_message.cc (right): https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:12: const int kRemoteSecurityKeyMessageControlCodeByteCount = sizeof(uint8_t); 1 instead sizeof(uint8_t) please https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:18: const int SecurityKeyMessage::kHeaderSizeBytes = sizeof(uint32_t); nit: replace sizeof(uint32_t) with 4. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:35: if (type_ == RemoteSecurityKeyMessageType::kUnitialized) { Do we also need to validate that it's in accepted range. E.g. should this accept type_=33? https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:43: payload_.append(++it, message_data.end()); You don't really gain anything by calling reserve() if append() is called only once. Also std::string uses random access iterator, so you could use "begin() + 1" here. But I think this would be more readable using substr(), i.e: payload_ = message_data.substr(1); https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_message.h (right): https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:12: #include "base/callback.h" callback_forward.h please https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:51: kConnectToSecurityKeyEnabledSessionMsg = 1, Please use MACRO_STYLE for these names. Chromium style guide says the following: Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. In enums that are actually enumerations (i.e. have multiple values), continue to use this style for consistency. Use kConstantNaming when using the "enum hack" to define a single constant, as you would for a const int or the like. const-style names are particularly confusing when you have enum class instead of just regular enum. Also, given that this is a class enum, and so the names are scoped, they don't need to be that long. Particularly you don't need "SecurityKey" in any of these names. E.g. they can be named as follows: INVALID CONNECT CONNECT_RESPONSE CONNECT_ERROR REQUEST REQUEST_RESPONSE REQUEST_ERROR UNKNOWN_COMMAND UNKNOWN_ERROR Note that the style guide doesn't allow to truncate Response to Rsp and Error to Err: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:102: typedef base::Callback<void(scoped_ptr<SecurityKeyMessage> message)> Callback; Move this above the constructor: https://google.github.io/styleguide/cppguide.html#Declaration_Order https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:102: typedef base::Callback<void(scoped_ptr<SecurityKeyMessage> message)> Callback; I don't think this callback should be part of this class. Maybe move it to the reader, or just outside of this class? https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:105: static const int kHeaderSizeBytes; move this above constructor. Also add "= 4" here - it doesn't need to be in the .cc file
PTAL! https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_message.cc (right): https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:12: const int kRemoteSecurityKeyMessageControlCodeByteCount = sizeof(uint8_t); On 2016/03/24 21:51:17, Sergey Ulanov wrote: > 1 instead sizeof(uint8_t) please Done. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:18: const int SecurityKeyMessage::kHeaderSizeBytes = sizeof(uint32_t); On 2016/03/24 21:51:17, Sergey Ulanov wrote: > nit: replace sizeof(uint32_t) with 4. Acknowledged. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:35: if (type_ == RemoteSecurityKeyMessageType::kUnitialized) { On 2016/03/24 21:51:17, Sergey Ulanov wrote: > Do we also need to validate that it's in accepted range. E.g. should this accept > type_=33? I thought about adding validation in but I don't think it is worth it as there is already a form of validation in the handler class (i.e. unknown messagea are rejected).. When we add messages or new error conditions, we need to update the enum and our validate function and I'd prefer this data type be simple and keep the validation in the handling code. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:43: payload_.append(++it, message_data.end()); On 2016/03/24 21:51:17, Sergey Ulanov wrote: > You don't really gain anything by calling reserve() if append() is called only > once. Also std::string uses random access iterator, so you could use "begin() + > 1" here. But I think this would be more readable using substr(), i.e: > payload_ = message_data.substr(1); Done. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_message.h (right): https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:12: #include "base/callback.h" On 2016/03/24 21:51:17, Sergey Ulanov wrote: > callback_forward.h please Done. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:51: kConnectToSecurityKeyEnabledSessionMsg = 1, On 2016/03/24 21:51:17, Sergey Ulanov wrote: > Please use MACRO_STYLE for these names. Chromium style guide says the following: > > Though the Google C++ Style Guide now says to use kConstantNaming for > enums, Chromium was written using MACRO_STYLE naming. In enums that are > actually enumerations (i.e. have multiple values), continue to use this > style for consistency. Use kConstantNaming when using the "enum hack" > to define a single constant, as you would for a const int or the like. > > const-style names are particularly confusing when you have enum class instead of > just regular enum. > > Also, given that this is a class enum, and so the names are scoped, they don't > need to be that long. Particularly you don't need "SecurityKey" in any of these > names. E.g. they can be named as follows: > > INVALID > CONNECT > CONNECT_RESPONSE > CONNECT_ERROR > REQUEST > REQUEST_RESPONSE > REQUEST_ERROR > UNKNOWN_COMMAND > UNKNOWN_ERROR > > Note that the style guide doesn't allow to truncate Response to Rsp and Error to > Err: https://google.github.io/styleguide/cppguide.html#General_Naming_Rules Done. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:102: typedef base::Callback<void(scoped_ptr<SecurityKeyMessage> message)> Callback; On 2016/03/24 21:51:18, Sergey Ulanov wrote: > I don't think this callback should be part of this class. Maybe move it to the > reader, or just outside of this class? The callback is used by the reader and handler (also channel but that may be going away). I don't mind moving it out of the class. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:102: typedef base::Callback<void(scoped_ptr<SecurityKeyMessage> message)> Callback; On 2016/03/24 21:51:17, Sergey Ulanov wrote: > Move this above the constructor: > https://google.github.io/styleguide/cppguide.html#Declaration_Order Acknowledged. https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.h:105: static const int kHeaderSizeBytes; On 2016/03/24 21:51:17, Sergey Ulanov wrote: > move this above constructor. Also add "= 4" here - it doesn't need to be in the > .cc file Done.
https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... File remoting/host/security_key/security_key_message.cc (right): https://codereview.chromium.org/1821393002/diff/40001/remoting/host/security_... remoting/host/security_key/security_key_message.cc:35: if (type_ == RemoteSecurityKeyMessageType::kUnitialized) { On 2016/03/28 19:44:26, joedow wrote: > On 2016/03/24 21:51:17, Sergey Ulanov wrote: > > Do we also need to validate that it's in accepted range. E.g. should this > accept > > type_=33? > > I thought about adding validation in but I don't think it is worth it as there > is already a form of validation in the handler class (i.e. unknown messagea are > rejected).. When we add messages or new error conditions, we need to update the > enum and our validate function and I'd prefer this data type be simple and keep > the validation in the handling code. I think we want to validate the value before the static_cast<>. static_cast<> behavior is unspecified when casting int type to enum. See http://stackoverflow.com/questions/18195312/what-happens-if-you-static-cast-i... This means that the compiler is allowed to crash. It's also allowed to truncate the higher bits, which means that the value may become valid after conversion. Alternatively you can change type of |type_| to int and assume that it's supposed to be validated elsewhere.
Added some enum value checking code. PTAL!
Fixed a non-windows build break. PTAL!
Ping!
lgtm https://codereview.chromium.org/1821393002/diff/100001/remoting/host/security... File remoting/host/security_key/security_key_message.cc (right): https://codereview.chromium.org/1821393002/diff/100001/remoting/host/security... remoting/host/security_key/security_key_message.cc:32: switch (value) { I think you can also format this code as follows: switch (value) { case static_cast<int>(RemoteSecurityKeyMessageType::CONNECT): case static_cast<int>(RemoteSecurityKeyMessageType::CONNECT_RESPONSE): ..... case static_cast<int>(RemoteSecurityKeyMessageType::INVALID): return static_cast<RemoteSecurityKeyMessageType>(value); default: return RemoteSecurityKeyMessageType::INVALID; } That way you need to list valid values only once.
Thanks! https://codereview.chromium.org/1821393002/diff/100001/remoting/host/security... File remoting/host/security_key/security_key_message.cc (right): https://codereview.chromium.org/1821393002/diff/100001/remoting/host/security... remoting/host/security_key/security_key_message.cc:32: switch (value) { On 2016/03/30 21:21:50, Sergey Ulanov wrote: > I think you can also format this code as follows: > > switch (value) { > case static_cast<int>(RemoteSecurityKeyMessageType::CONNECT): > case static_cast<int>(RemoteSecurityKeyMessageType::CONNECT_RESPONSE): > ..... > case static_cast<int>(RemoteSecurityKeyMessageType::INVALID): > return static_cast<RemoteSecurityKeyMessageType>(value); > > default: > return RemoteSecurityKeyMessageType::INVALID; > } > > That way you need to list valid values only once. Done.
The CQ bit was checked by joedow@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/1821393002/#ps120001 (title: "Updating enum translation function per feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1821393002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1821393002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Adding the message definitions for the remote_security_key STDIO communication classes. This is the first of several CLs which will introduce the classes needed by the remote_security_key_process to communicate with security key enabled tools. BUG=584463 ========== to ========== Adding the message definitions for the remote_security_key STDIO communication classes. This is the first of several CLs which will introduce the classes needed by the remote_security_key_process to communicate with security key enabled tools. BUG=584463 Committed: https://crrev.com/fd9d9f4e926a8471e2bdeb621e0916e4edba513c Cr-Commit-Position: refs/heads/master@{#384128} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/fd9d9f4e926a8471e2bdeb621e0916e4edba513c Cr-Commit-Position: refs/heads/master@{#384128} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
