Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upGet rid of kdialog/zenity in Dom0 and replace it with tray notification #877
Comments
marmarek
added this to the Release 2 milestone
Mar 8, 2015
marmarek
added
the
bug
label
Mar 8, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 3 Jul 2014 00:16 UTC
The same for "VM hasn't shutdown, wait or kill" dialog box -- convert it to a try notification. Especially that it really is only a hint based on guessing...
|
Comment by joanna on 3 Jul 2014 00:16 UTC |
marmarek
added
C: core
P: minor
labels
Mar 8, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Modified by joanna on 3 Jul 2014 00:16 UTC |
marmarek
added
P: major
and removed
P: minor
labels
Mar 8, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by marmarek on 3 Jul 2014 00:48 UTC
Regarding "VM hasn't shutdown, wait or kill" - what about buttons on that dialog? Perhaps we should simply get rid of that message and let the user spot hanged VMs him/herself?
|
Comment by marmarek on 3 Jul 2014 00:48 UTC |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by marmarek on 3 Jul 2014 02:10 UTC
Problem from ticket description solved here:
http://git.qubes-os.org/?p=marmarek/core-admin.git;a=commit;h=6ce4028033a2f6bf0e4492791122ee8ede4e6761
|
Comment by marmarek on 3 Jul 2014 02:10 UTC |
marmarek
self-assigned this
Mar 8, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 3 Jul 2014 08:12 UTC
I think it's still useful to bring the user's attention that something is apparently wrong (the VM hasn't shutdown within expected time period). But we don't need to duplicate the buttons of the manager in the notification -- the user, seeing the message could then decide if they want to wait or just go and kill it.
|
Comment by joanna on 3 Jul 2014 08:12 UTC |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
Modified by joanna on 3 Jul 2014 09:15 UTC |
marmarek
changed the title from
The no-memory to start DispVM message should be a tray notification
to
Get rid of kdialog/zenity in Dom0 and replace it with tray notification
Mar 8, 2015
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 3 Jul 2014 09:23 UTC
Not only will that be consistent, more user friendly, but also make the code simpler and nicer:
[qubes-src](user@qubes)$ grep -R kdialog core-admin* gui-*
core-admin/dispvm/qubes-update-dispvm-savefile-with-progress.sh:if type kdialog &> /dev/null; then
core-admin/dispvm/qubes-update-dispvm-savefile-with-progress.sh: ref=`kdialog --title="Updating default DispVM savefile" \
core-admin/dispvm/qfile-daemon-dvm: if os.path.exists('/usr/bin/kdialog'):
core-admin/dispvm/qfile-daemon-dvm: subprocess.call(['--sorry', errmsg]('/usr/bin/kdialog',))
core-admin-linux/dom0-updates/qubes-dom0-update: kdialog --sorry "$message1<br/>$message2"
grep: gui-agent-linux/xf86-input-mfndev/ltmain.sh: No such file or directory
gui-daemon/gui-daemon/xside.c: int use_kdialog; /* use kdialog for prompts (default on KDE) or zenity (default on non-KDE) */
gui-daemon/gui-daemon/xside.c:#define KDIALOG_PATH "/usr/bin/kdialog"
gui-daemon/gui-daemon/xside.c: if (g->use_kdialog) {
gui-daemon/gui-daemon/xside.c: execlp(KDIALOG_PATH, "kdialog", "--sorry", message, (char*)NULL);
gui-daemon/gui-daemon/xside.c: fprintf(stderr, "Problems executing %s ?\n", g->use_kdialog ? "kdialog" : "zenity");
gui-daemon/gui-daemon/xside.c: if (g->use_kdialog) {
gui-daemon/gui-daemon/xside.c: execlp(KDIALOG_PATH, "kdialog", "--dontagain", dontagain_param, "--no-label", "Terminate", "--yes-label", "Ignore", "--warningyesno", text, (char*)NULL);
gui-daemon/gui-daemon/xside.c: execlp(KDIALOG_PATH, "kdialog", "--dontagain", dontagain_param, "--warningyesno", text, (char*)NULL);
gui-daemon/gui-daemon/xside.c: if (!g->use_kdialog) {
gui-daemon/gui-daemon/xside.c: fprintf(stderr, "Problems executing %s ?\n", g->use_kdialog ? "kdialog" : "zenity");
gui-daemon/gui-daemon/xside.c: g->use_kdialog = 1;
gui-daemon/gui-daemon/xside.c: g->use_kdialog = 0;
gui-daemon/gui-daemon/xside.c: g->use_kdialog ? KDIALOG_PATH : ZENITY_PATH,
gui-daemon/gui-daemon/xside.c: g->use_kdialog ? "--yesnocancel" : "--question --text",
gui-daemon/gui-daemon/xside.c: fprintf(stderr, "Problems executing %s ?\n", g->use_kdialog ? "kdialog" : "zenity");
gui-daemon/gui-daemon/xside.c: g->use_kdialog ? KDIALOG_PATH : ZENITY_PATH,
gui-daemon/gui-daemon/xside.c: g->use_kdialog ? "--sorry" : "--error --text ",
gui-daemon/gui-daemon/xside.c: g->use_kdialog ? KDIALOG_PATH : ZENITY_PATH,
gui-daemon/gui-daemon/xside.c: g->use_kdialog ? "--sorry" : "--error --text ",
The ask_whether_flooding() in xside requires some further discussion, but other uses in xside (show_error_message, etc) present no problem IMHO.
|
Comment by joanna on 3 Jul 2014 09:23 UTC The ask_whether_flooding() in xside requires some further discussion, but other uses in xside (show_error_message, etc) present no problem IMHO. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 3 Jul 2014 09:28 UTC
The ask_whether_flooding() is a bit controversial because in the last 4+ years of using Qubes I got this prompt several times and I have 'always' clicked yes :) In fact I don't believe anybody would be willing to click 'no' and risk losing unsaved work only because some app, such as Office, suddenly created too many windows...IMHO this whole anti-DoS test is an example of a wrong UI.
|
Comment by joanna on 3 Jul 2014 09:28 UTC |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 3 Jul 2014 09:31 UTC
So, rather than having the annoying and never-expected-to-be-actually-used-by-user ask_whether_flooding() we should rather have a way to ensure that the user will always be able to kill the offending VM in case it started to create so many windows. This problem is related to another, much more practical GUI DoS problem associated with frameless window support, and potentially also with focus management.
|
Comment by joanna on 3 Jul 2014 09:31 UTC |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by marmarek on 3 Jul 2014 14:39 UTC
I don't think its a good idea to replace all zenity/kdialog calls with tray notifies. Especially questions should stay as there are, but some errors also. Otherwise can went unnoticed and the user might get impression that "nothing happened". For example some long operations, like dom0 updates.
But indeed, most of them can be replaced with tray notifications. Regarding gui-daemon, I see only few types of errors reported to the user:
- Invalid (possibly malicious) message from VM.
- Protocol version mismatch.
- Various errors during clipboard handling.
- ask_whether_flooding
I think 1 and 2 should definitely stay as kdialog/zenity, 3 should be definitely converted to tray notifications. Indeed a way to kill (or at least pause?) a VM would solve point 4, but we don't currently have any. Some keyboard shortcut? The other idea would be to replace ask_whether_flooding mechanism with some rate limit + tray notification when the limit is hit. But I'd prefer to have more generic mechanism to handle misbehaving VM.
|
Comment by marmarek on 3 Jul 2014 14:39 UTC But indeed, most of them can be replaced with tray notifications. Regarding gui-daemon, I see only few types of errors reported to the user:
I think 1 and 2 should definitely stay as kdialog/zenity, 3 should be definitely converted to tray notifications. Indeed a way to kill (or at least pause?) a VM would solve point 4, but we don't currently have any. Some keyboard shortcut? The other idea would be to replace ask_whether_flooding mechanism with some rate limit + tray notification when the limit is hit. But I'd prefer to have more generic mechanism to handle misbehaving VM. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 3 Jul 2014 18:01 UTC
Long operations, such as Dom0 updates download, should be first signalized by a tray notification, and later by status icon in the manager (e.g. animated updating icon).
Regarding questions: the current qrexec policy enforcer prompt ("Yes, No, Yes to All") should, for now (i.e. R2), stay as it is. But in the future we don't want to force the user to have to answer such policy questions, because we know that the user always answers YES.
Regarding guid anti-DOS prompt -- I think this is totally useless -- not only for reasons explained above (user will never choose to kill the VM based on some suddenly-appearing dialog box -- if at all, then only based on the actual observed behaviour), but also because a trivial GUI DoS that does not require to trigger this alert is to just create one big window with override_redirect. Because we don't handle this (very practical DoS GUI attack), I don't see why should we be handling this exotic window flood...
Regarding reporting errors -- some time ago we decided to report all errors related to VM startup/agent connection via: tray notification + yellow triangle in the manager status column.
|
Comment by joanna on 3 Jul 2014 18:01 UTC Regarding questions: the current qrexec policy enforcer prompt ("Yes, No, Yes to All") should, for now (i.e. R2), stay as it is. But in the future we don't want to force the user to have to answer such policy questions, because we know that the user always answers YES. Regarding guid anti-DOS prompt -- I think this is totally useless -- not only for reasons explained above (user will never choose to kill the VM based on some suddenly-appearing dialog box -- if at all, then only based on the actual observed behaviour), but also because a trivial GUI DoS that does not require to trigger this alert is to just create one big window with override_redirect. Because we don't handle this (very practical DoS GUI attack), I don't see why should we be handling this exotic window flood... Regarding reporting errors -- some time ago we decided to report all errors related to VM startup/agent connection via: tray notification + yellow triangle in the manager status column. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by marmarek on 4 Jul 2014 02:37 UTC
Both DispVM messages converted to tray notifications:
http://git.qubes-os.org/?p=marmarek/core-admin.git;a=commit;h=c10909e9f954f10b1ed77d579aea4c5c02b87738
Removed windows count limit, converted clipboard errors to tray notification:
http://git.qubes-os.org/?p=marmarek/gui-daemon.git;a=commit;h=80356df2419c7ac376c230246f0bc649c70e6217
http://git.qubes-os.org/?p=marmarek/gui-daemon.git;a=commit;h=76cb32831c7830b6248dd4192213d47ce3ce159f
Protocol mismatch and suspicious command from VM left as kdialog. There are plenty reasons why '''serious errors''' should be handled this way, at least:
- Hard to miss them. Tray notifications expire after few seconds, so unless user actively search what is wrong, he/she will not know why application didn't started/was killed - especially in case when some specific user activity is required to fix the problem (update template/dom0).
- Ability to copy message. Especially important for protocol mismatch - there is instruction what to do. It isn't possible to copy that two commands from tray notification neither tooltip in manager.
- Pause further GUI activity. When VM is misbehaving, let stop processing further messages until user decide what to do. Do not let the VM to continue trying to exploit/crash guid, neither fill the screen with tray notifications...
BTW In any case, when guid will be terminated, it will be marked in qubes manager with at least yellow dot (or in case protocol version mismatch - also with yellow rectangle as guid startup failure). So even when user dismiss kdialog message, there will be some info in qubes manager.
So I propose to close the ticket in current shape.
|
Comment by marmarek on 4 Jul 2014 02:37 UTC Removed windows count limit, converted clipboard errors to tray notification: Protocol mismatch and suspicious command from VM left as kdialog. There are plenty reasons why '''serious errors''' should be handled this way, at least:
BTW In any case, when guid will be terminated, it will be marked in qubes manager with at least yellow dot (or in case protocol version mismatch - also with yellow rectangle as guid startup failure). So even when user dismiss kdialog message, there will be some info in qubes manager. So I propose to close the ticket in current shape. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
marmarek
Mar 8, 2015
Member
Comment by joanna on 4 Jul 2014 09:35 UTC
Ok, we will further unify error reporting in R3 :) Closing for now.
|
Comment by joanna on 4 Jul 2014 09:35 UTC |
marmarek commentedMar 8, 2015
Reported by joanna on 2 Jul 2014 18:15 UTC
In order to be consistent with how we display notification for other errors (even the same error, but for normal AppVMs, is displayed via tray notification).
The actual message for easy grepping:
"Not enough memory to create DVM. Terminate some appVM and retry."
Migrated-From: https://wiki.qubes-os.org/ticket/877