Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1954623005:
Add Xorg+dummy as alternative for Xvfb (Closed)
DescriptionAdd Xorg+dummy as alternative for Xvfb
Setting the CHROME_REMOTE_DESKTOP_USE_XORG environment variable will
cause the curtain-mode script to launch Xorg with void input and dummy
output instead of Xvfb.
BUG=596125
Committed: https://crrev.com/aede46c4efcd218750985f1a0fcc29342aa1e400
Cr-Commit-Position: refs/heads/master@{#392775}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address nits #
Total comments: 6
Patch Set 3 : Replace "Xvfb" with "virtual" and move Xorg config to its own function #
Total comments: 8
Patch Set 4 : Address final comments #Messages
Total messages: 22 (6 generated)
rkjnsn@chromium.org changed reviewers: + jamiewalch@chromium.org, lambroslambrou@chromium.org
Please review this initial implementation of using Xorg. You will need both the dummy video driver and void input driver installed to test: sudo apt-get install xserver-xorg-video-dummy sudo apt-get install xserver-xorg-input-void This currently doesn't verify the output is dummy, but it does confirm that the only input connected is the "Chrome Remote Desktop Input" void input we specify, which should at least keep the host from connecting to the user's main session by accident when curtain-mode enforcement is enabled. Style-wise, the Xorg config template is currently in the _launch_xorg method. I wasn't sure if it would make sense to move it to the top of the script as a global constant.
FWIW, I think the XOrg config is fine where it is. I'll let Lambros give the final sign-off on this. I just have a couple of nits. https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... remoting/host/curtain_mode_linux.cc:104: return (found_xvfb_mouse && found_xvfb_keyboard) != found_crd_void_input Why are you using != here instead of ||? It makes the code harder to read, and I don't think we care if we find both. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:47: # Xorg using the dummy display driver and void input device. Add instructions here on how to install dummy and void (you can remove them from the CL description if you like). https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:416: ' VideoRam 1048576\n' I think it's worth defining named constants for this (and other) magic values in the config file, with a brief description of why they were chosen. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:447: config_file.close() Would using "with" be more Pythonic than explicitly closing the file here? https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:459: # can pass environment variables to Xorg like they could for Xvfb. I don't think we need this TODO. It seems unlikely we'll ever do it unless it proves to be a problem for end-users. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:466: # log file How does specifying the logfile as /dev/null log to stderr? I'd have thought it effectively disabled logging.
https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... remoting/host/curtain_mode_linux.cc:104: return (found_xvfb_mouse && found_xvfb_keyboard) != found_crd_void_input On 2016/05/07 00:05:47, Jamie wrote: > Why are you using != here instead of ||? It makes the code harder to read, and I > don't think we care if we find both. My initial thought was that it should be xor since we'll either be running on one or the other, but I agree it shouldn't be a problem to use ||, so I'll switch if you think it's more readable that way. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:47: # Xorg using the dummy display driver and void input device. On 2016/05/07 00:05:47, Jamie wrote: > Add instructions here on how to install dummy and void (you can remove them from > the CL description if you like). It's obvious now that you mention it. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:416: ' VideoRam 1048576\n' On 2016/05/07 00:05:47, Jamie wrote: > I think it's worth defining named constants for this (and other) magic values in > the config file, with a brief description of why they were chosen. That requires me to have a good reason for why they were chosen. :) Will do. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:447: config_file.close() On 2016/05/07 00:05:47, Jamie wrote: > Would using "with" be more Pythonic than explicitly closing the file here? Probably. Will do. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:459: # can pass environment variables to Xorg like they could for Xvfb. On 2016/05/07 00:05:47, Jamie wrote: > I don't think we need this TODO. It seems unlikely we'll ever do it unless it > proves to be a problem for end-users. Acknowledged. https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:466: # log file On 2016/05/07 00:05:47, Jamie wrote: > How does specifying the logfile as /dev/null log to stderr? I'd have thought it > effectively disabled logging. Ah, yes. I was worried this might not be clear. By default, Xorg logs to both a file at level 3 and stderr at level 0. This is disabling logging to the file, and bumping the stderr logging (which is already enabled) to level 3 so it mirrors what would be written to the log file. I'll try to expand a bit more in the comment.
https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... remoting/host/curtain_mode_linux.cc:104: return (found_xvfb_mouse && found_xvfb_keyboard) != found_crd_void_input On 2016/05/09 16:58:33, rkjnsn wrote: > On 2016/05/07 00:05:47, Jamie wrote: > > Why are you using != here instead of ||? It makes the code harder to read, and > I > > don't think we care if we find both. > > My initial thought was that it should be xor since we'll either be running on > one or the other, but I agree it shouldn't be a problem to use ||, so I'll > switch if you think it's more readable that way. Xor is unusual enough that it's not immediately clear that "!=" is the non-bitwise form of "^", which is mostly what makes this hard to read. I think "^" would work, and would be clearer, but I think "||" is better.
https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/curtain_mode_... remoting/host/curtain_mode_linux.cc:48: bool CurtainModeLinux::IsXvfbSession() { Rename this to avoid mentioning Xvfb, maybe IsVirtualSession() ?
Addressed nits (and possibly created a few more).
lgtm https://codereview.chromium.org/1954623005/diff/20001/remoting/host/curtain_m... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/20001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:49: // Try to identify an Xvfb session. There's no way to query what X server we Should probably remove all "Xvfb" references (as well as renaming this method to indicate we're testing for a virtual session not necessarily backed by Xvfb). https://codereview.chromium.org/1954623005/diff/20001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:77: HOST_LOG << "Non Xvfb mouse found: " << device_info->name; Maybe "Non-virtual.." or "External.." or "Physical.." ? https://codereview.chromium.org/1954623005/diff/20001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:405: # This causes X to load the default GLX module, even if a proprietary Optional suggestion: Declare this config as a Python global string outside this function, using literal """...""" docstring notation. For example: XORG_CONF = """\ blah blah more blah """ This will: * reduce the indentation of the config * shorten this function, helping readability * avoid the need for embedded '\n' characters Unfortunately, it won't let you intersperse Python comments.
https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:466: # log file On 2016/05/09 16:58:33, rkjnsn wrote: > On 2016/05/07 00:05:47, Jamie wrote: > > How does specifying the logfile as /dev/null log to stderr? I'd have thought > it > > effectively disabled logging. > > Ah, yes. I was worried this might not be clear. By default, Xorg logs to both a > file at level 3 and stderr at level 0. This is disabling logging to the file, > and bumping the stderr logging (which is already enabled) to level 3 so it > mirrors what would be written to the log file. I'll try to expand a bit more in > the comment. I completely missed the -verbose flag. Please move it to a separate line so that you've got one option per line as per the other options. https://codereview.chromium.org/1954623005/diff/20001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:435: ' VertRefresh 0.1\n' HorizSync and VertRefresh should probably also use named constants with a rationale for why these values were chosen, since it's not obvious.
Patchset #3 (id:40001) has been deleted
On 2016/05/10 00:30:16, Lambros wrote: > … > https://codereview.chromium.org/1954623005/diff/20001/remoting/host/linux/lin... > remoting/host/linux/linux_me2me_host.py:405: # This causes X to load the default > GLX module, even if a proprietary > Optional suggestion: > Declare this config as a Python global string outside this function, using > literal """...""" docstring notation. For example: > XORG_CONF = """\ > blah > blah > more blah > """ > > This will: > * reduce the indentation of the config > * shorten this function, helping readability > * avoid the need for embedded '\n' characters > Unfortunately, it won't let you intersperse Python comments. On 2016/05/10 00:37:09, Jamie wrote: > … > https://codereview.chromium.org/1954623005/diff/20001/remoting/host/linux/lin... > remoting/host/linux/linux_me2me_host.py:435: ' VertRefresh 0.1\n' > HorizSync and VertRefresh should probably also use named constants with a > rationale for why these values were chosen, since it's not obvious. I'm not sure it makes sense to make HorizSync and VirtRefresh as separate constants, as, unlike the video RAM, they aren't things the user might want to tweak, and they cannot be tweaked independently from each other and the Modeline template without breaking the configuration. Thus, I think it makes more sense to have them placed in the config file so the have the appropriate context. I also find it more readable to have the config file template appear where it is being formatted, so everything is in one place, and the Modeline template is next to the rest of the config file. My latest patch set thus attempts to address the above concerns with a different approach: a new global function for generating an Xorg config for a given list of sizes. This: * Puts the config template toward the top near the rest of the constants * Keeps the various pieces of the config file in one place * Shrinks the _launch_xorg method, making it more readable * Allows me to keep the interspersed commentary, which has been expanded to explain the constants Jamie mentioned. Let me know what you think of this approach. Thanks! https://codereview.chromium.org/1954623005/diff/20001/remoting/host/curtain_m... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/20001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:49: // Try to identify an Xvfb session. There's no way to query what X server we On 2016/05/10 00:30:15, Lambros wrote: > Should probably remove all "Xvfb" references (as well as renaming this method to > indicate we're testing for a virtual session not necessarily backed by Xvfb). Done. https://codereview.chromium.org/1954623005/diff/20001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:77: HOST_LOG << "Non Xvfb mouse found: " << device_info->name; On 2016/05/10 00:30:15, Lambros wrote: > Maybe "Non-virtual.." or "External.." or "Physical.." ? > Done.
Patchset #3 (id:60001) has been deleted
Looks good, thanks. I'll defer to Lambros for the final sign-off.
lgtm https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:26: // Returns true if the host is running under an virtual session. nit: ..a virtual.. https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:39: bool activated = IsVirtualSession(); optional: remove |activated|, and write this as: if (IsVirtualSession()) { return true; } LOG(ERROR)... return false; https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:49: // Try to identify an virtual session. Since there's no way to tell from the ...a virtual... https://codereview.chromium.org/1954623005/diff/80001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/80001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:52: # sudo apt-get install xserver-xorg-input-void This is OK for now, but when we make Xorg+dummy the default, we should add these as Debian package dependencies.
https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... File remoting/host/curtain_mode_linux.cc (right): https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:26: // Returns true if the host is running under an virtual session. On 2016/05/10 21:06:56, Lambros wrote: > nit: ..a virtual.. > Acknowledged. https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:39: bool activated = IsVirtualSession(); On 2016/05/10 21:06:56, Lambros wrote: > optional: > remove |activated|, and write this as: > > if (IsVirtualSession()) { > return true; > } > > LOG(ERROR)... > return false; Acknowledged, though I switched it so the error case is the early return, since I think that's a more common idiom: if (!IsVirtualSession()) { LOG(ERROR) << "…"; return false; } return true; https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... remoting/host/curtain_mode_linux.cc:49: // Try to identify an virtual session. Since there's no way to tell from the On 2016/05/10 21:06:56, Lambros wrote: > ...a virtual... Acknowledged. https://codereview.chromium.org/1954623005/diff/80001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1954623005/diff/80001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:52: # sudo apt-get install xserver-xorg-input-void On 2016/05/10 21:06:57, Lambros wrote: > This is OK for now, but when we make Xorg+dummy the default, we should add these > as Debian package dependencies. Certainly. Should I add a TODO?
On 2016/05/10 21:38:03, rkjnsn wrote: > https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... > File remoting/host/curtain_mode_linux.cc (right): > > https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... > remoting/host/curtain_mode_linux.cc:26: // Returns true if the host is running > under an virtual session. > On 2016/05/10 21:06:56, Lambros wrote: > > nit: ..a virtual.. > > > > Acknowledged. > > https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... > remoting/host/curtain_mode_linux.cc:39: bool activated = IsVirtualSession(); > On 2016/05/10 21:06:56, Lambros wrote: > > optional: > > remove |activated|, and write this as: > > > > if (IsVirtualSession()) { > > return true; > > } > > > > LOG(ERROR)... > > return false; > > Acknowledged, though I switched it so the error case is the early return, since > I think that's a more common idiom: > > if (!IsVirtualSession()) { > LOG(ERROR) << "…"; > return false; > } > > return true; > > https://codereview.chromium.org/1954623005/diff/80001/remoting/host/curtain_m... > remoting/host/curtain_mode_linux.cc:49: // Try to identify an virtual session. > Since there's no way to tell from the > On 2016/05/10 21:06:56, Lambros wrote: > > ...a virtual... > > Acknowledged. > > https://codereview.chromium.org/1954623005/diff/80001/remoting/host/linux/lin... > File remoting/host/linux/linux_me2me_host.py (right): > > https://codereview.chromium.org/1954623005/diff/80001/remoting/host/linux/lin... > remoting/host/linux/linux_me2me_host.py:52: # sudo apt-get install > xserver-xorg-input-void > On 2016/05/10 21:06:57, Lambros wrote: > > This is OK for now, but when we make Xorg+dummy the default, we should add > these > > as Debian package dependencies. > > Certainly. Should I add a TODO? Good idea :)
The CQ bit was checked by rkjnsn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1954623005/#ps100001 (title: "Address final comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954623005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954623005/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add Xorg+dummy as alternative for Xvfb Setting the CHROME_REMOTE_DESKTOP_USE_XORG environment variable will cause the curtain-mode script to launch Xorg with void input and dummy output instead of Xvfb. BUG=596125 ========== to ========== Add Xorg+dummy as alternative for Xvfb Setting the CHROME_REMOTE_DESKTOP_USE_XORG environment variable will cause the curtain-mode script to launch Xorg with void input and dummy output instead of Xvfb. BUG=596125 Committed: https://crrev.com/aede46c4efcd218750985f1a0fcc29342aa1e400 Cr-Commit-Position: refs/heads/master@{#392775} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/aede46c4efcd218750985f1a0fcc29342aa1e400 Cr-Commit-Position: refs/heads/master@{#392775} |
