Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11140031:
[Chromoting] Add a Linux command-line app that can register and start a host. (Closed)
|
Created:
8 years, 2 months ago by simonmorris Modified:
8 years, 2 months ago Reviewers:
Sergey Ulanov 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, rmsousa+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] Add a Linux command-line app that can register and start a host.
BUG=155431
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162331
Patch Set 1 #
Total comments: 20
Patch Set 2 : Review. #
Total comments: 12
Patch Set 3 : Review. #Patch Set 4 : Build remoting_start_host only on Linux. #Messages
Total messages: 12 (0 generated)
ptal
http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host.cc File remoting/host/setup/start_host.cc (right): http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:4: add comment to describe what this file is for. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:21: bool fail = true; maybe store exit code for consistency with our Me2Me host code. In either case you need g_ prefix for global variables. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:24: MessageLoop* message_loop = NULL; g_message_loop Or maybe put everything in a class - then you wouldn't need any globals. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:74: if (host_name.empty() || auth_code.empty()) { Why hostname is a required parameter? http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:74: if (host_name.empty() || auth_code.empty()) { It may not always be secure to pass authentication code in command line. Maybe ask it in the same you ask for PIN. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:75: std::cerr << "Usage: " << argv[0] Here and below - style guide discourages usage of C++ streams for IO - use [f]printf()/scanf() instead: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:76: << " -name=<hostname> -code=<auth-code> [-pin=<PIN>]\n"; I think it should be --name, --code and --pin http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:93: std::cerr << "You entered different PINs.\n"; better UX would be to ask the PIN again instead of quitting here. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:108: message_loop = new MessageLoop(MessageLoop::TYPE_UI); This object is never freed. I think it's more common to allocate MessageLoops on the stack instead of on the heap: MessageLoop message_loop; g_message_loop = &message_loop; ... Run the loop ... g_message_loop = NULL; http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:114: // are destroyed before the IO thread is stopped. nit: Alternatively you could just reset url_request_context_getter and host_starter explicitly. IMHO that would make this code more readable,
http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host.cc File remoting/host/setup/start_host.cc (right): http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:4: On 2012/10/15 18:25:30, sergeyu wrote: > add comment to describe what this file is for. Done. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:21: bool fail = true; On 2012/10/15 18:25:30, sergeyu wrote: > maybe store exit code for consistency with our Me2Me host code. In either case > you need g_ prefix for global variables. I'm not sure an exit code would be useful. Added a g_ prefix. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:24: MessageLoop* message_loop = NULL; On 2012/10/15 18:25:30, sergeyu wrote: > g_message_loop > > Or maybe put everything in a class - then you wouldn't need any globals. Added a g_ prefix. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:74: if (host_name.empty() || auth_code.empty()) { On 2012/10/15 18:25:30, sergeyu wrote: > Why hostname is a required parameter? For simplicity. The host has to have a name. If the command line didn't specify a hostname, would it be better to use the OS hostname, or to ask the user to type one in? http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:74: if (host_name.empty() || auth_code.empty()) { On 2012/10/15 18:25:30, sergeyu wrote: > It may not always be secure to pass authentication code in command line. Maybe > ask it in the same you ask for PIN. Done. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:75: std::cerr << "Usage: " << argv[0] On 2012/10/15 18:25:30, sergeyu wrote: > Here and below - style guide discourages usage of C++ streams for IO - use > [f]printf()/scanf() instead: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Stream... Done. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:76: << " -name=<hostname> -code=<auth-code> [-pin=<PIN>]\n"; On 2012/10/15 18:25:30, sergeyu wrote: > I think it should be --name, --code and --pin Done. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:93: std::cerr << "You entered different PINs.\n"; On 2012/10/15 18:25:30, sergeyu wrote: > better UX would be to ask the PIN again instead of quitting here. Done. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:108: message_loop = new MessageLoop(MessageLoop::TYPE_UI); On 2012/10/15 18:25:30, sergeyu wrote: > This object is never freed. I think it's more common to allocate MessageLoops on > the stack instead of on the heap: > > MessageLoop message_loop; > g_message_loop = &message_loop; > > ... Run the loop ... > > g_message_loop = NULL; Done. http://codereview.chromium.org/11140031/diff/1/remoting/host/setup/start_host... remoting/host/setup/start_host.cc:114: // are destroyed before the IO thread is stopped. On 2012/10/15 18:25:30, sergeyu wrote: > nit: Alternatively you could just reset url_request_context_getter and > host_starter explicitly. IMHO that would make this code more readable, Done.
lgtm, but see my nits http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... File remoting/host/setup/start_host.cc (right): http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:23: bool g_fail = true; better to replace it with "g_started = false;". It seems to wrong to set fail flag to true, even before we started. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:47: int result = scanf(format.c_str(), &str[0]); Why not just use fgets() here, instead of scanf? Also, AFAIK scanf() would fail to compile on windows - you need to use scanf_s() instead. fgets() doesn't have that problem. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:103: fprintf(stdout, "Enter a six-digit PIN: "); I think you also need to fflush(stdout) in case stdout is buffered. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:111: fprintf(stdout, "Enter the same PIN again: "); fflush(stdout) http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:127: fprintf(stdout, "Enter an authorization code: "); fflush(stdout) http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:136: MessageLoop message_loop(MessageLoop::TYPE_UI); Why do you need to specify TYPE_UI here? If you need UI thread, then this should be MessageLoopForUI object, but it's better to avoid it since it would add dependency on UI libraries in a command-line tool.
fyi http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... File remoting/host/setup/start_host.cc (right): http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:23: bool g_fail = true; On 2012/10/16 00:06:52, sergeyu wrote: > better to replace it with "g_started = false;". It seems to wrong to set fail > flag to true, even before we started. Done. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:47: int result = scanf(format.c_str(), &str[0]); On 2012/10/16 00:06:52, sergeyu wrote: > Why not just use fgets() here, instead of scanf? > Also, AFAIK scanf() would fail to compile on windows - you need to use scanf_s() > instead. fgets() doesn't have that problem. Done. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:103: fprintf(stdout, "Enter a six-digit PIN: "); On 2012/10/16 00:06:52, sergeyu wrote: > I think you also need to fflush(stdout) in case stdout is buffered. Done. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:111: fprintf(stdout, "Enter the same PIN again: "); On 2012/10/16 00:06:52, sergeyu wrote: > fflush(stdout) Done. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:127: fprintf(stdout, "Enter an authorization code: "); On 2012/10/16 00:06:52, sergeyu wrote: > fflush(stdout) Done. http://codereview.chromium.org/11140031/diff/5001/remoting/host/setup/start_h... remoting/host/setup/start_host.cc:136: MessageLoop message_loop(MessageLoop::TYPE_UI); On 2012/10/16 00:06:52, sergeyu wrote: > Why do you need to specify TYPE_UI here? If you need UI thread, then this should > be MessageLoopForUI object, but it's better to avoid it since it would add > dependency on UI libraries in a command-line tool. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11140031/9001
Sorry for I got bad news for ya. Compile failed with a clobber build. Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
I've moved the remoting_start_host target to the Linux-only section of remoting.gyp, because the disable-echo code is Linux-only, and we don't currently need this app on any other OS. PTAL.
On 2012/10/16 16:28:28, simonmorris wrote: > I've moved the remoting_start_host target to the Linux-only section of > remoting.gyp, because the disable-echo code is Linux-only, and we don't > currently need this app on any other OS. > > PTAL. ping
On 2012/10/17 00:34:43, simonmorris wrote: > On 2012/10/16 16:28:28, simonmorris wrote: > > I've moved the remoting_start_host target to the Linux-only section of > > remoting.gyp, because the disable-echo code is Linux-only, and we don't > > currently need this app on any other OS. > > > > PTAL. > > ping LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/11140031/13001
Change committed as 162331 |
