Make maximum clipboard buffer lenght configurable #150
Conversation
alimirjamali
force-pushed
the
issue-9296
branch
from
5e17fb1
to
b0cb18f
Compare
alimirjamali
force-pushed
the
issue-9296
branch
7 times, most recently
from
98a0c57
to
3b1dedd
Compare
alimirjamali
force-pushed
the
issue-9296
branch
from
3b1dedd
to
0265918
Compare
alimirjamali
force-pushed
the
issue-9296
branch
from
0265918
to
3a15fee
Compare
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024102404-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2024091704-4.3&flavor=update
Failed tests30 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/112766#dependencies 199 fixed
Unstable tests
|
Exactly. I thought about it as well. Most use cases would be reading the metadata rather than writing to it. I wanted to avoid dependency on any external JSON parsing library for C language to support full JSON. In the end, all of xside.c should be replaced by Demi's rewrite of GUI daemons in Rust? There is a mention of clipboard files in qubes-doc. But they are for end-user. I still believe that it might be a good place to document the metadata format. |
Yes. I guess instead of an error message, p.s.: I will use |
TBH, I'm not a fan of mixing content of the loaded file with info about loading process itself. And also, the missing file needs special treatment anyway (compared to other errors on opening the file like permission denied) - missing file is okay on copy, and also kinda expected on some paste (if user didn't copied anything before). But for example permission error or io error are not okay (they should not be treated same as missing file). So, maybe in case of missing file report it via a return value from the function (making it more than a bool) and log message about it only in the paste path? But for other errors log errors directly from the loading function. Or, add a parameter whether missing file message should be logged? |
One day maybe, but not anytime soon. Anyway, it's okay to support just subset of JSON (as you said - it's mostly xside who write the file, but there are few where others write it too - for example copying dom0 clipboard via the clipboard widget). But it needs to be documented what that subset is. |
I just realized that fgets includes |
The previous code reported non-existent I am doing some improvements and will send push them soon. |
|
@marmarek metadata parsing fixes and improvement are pushed in the new commit to this PR (hopefully it would make the review easier).
I will do this as well. But maybe it would better to finalize this PR and then do that? |
|
p.s. It appears that it still has some issues. I am working on it |
|
Found the error. In key, value parsing formatting string, I did not include underline as a valid character for key. p.s. And it turns out that underline and hyphen should be allowed for vmname too |
|
I am adding a special case if user tries to press ctrl+shift+c in a qube where the clipboard is empty. This can be handled with the new metadata (copy_action=true, successful=true, cleared=true, sent_size=0) |
alimirjamali
force-pushed
the
issue-9296
branch
from
c853445
to
83b6395
Compare
|
I made another small adjustment. If |
alimirjamali
force-pushed
the
issue-9296
branch
from
83b6395
to
e1e6026
Compare
|
|
||
| file = fopen(QUBES_CLIPBOARD_FILENAME ".metadata", "r"); | ||
| if (!file) { | ||
| if (logging) |
There was a problem hiding this comment.
Errors other than ENOENT should result in a logged message even on copy operation. Similar comment applies to other messages below messages below. Generally, expected situation should not be reported as an error but others should get logged, and expected case is either fie exists and is correctly parsed, or file doesn't exist yet on the first copy.
There was a problem hiding this comment.
Errors other than ENOENT should result in a logged message even on copy operation. Similar comment applies to other messages below messages below. Generally, expected situation should not be reported as an error but others should get logged, and expected case is either fie exists and is correctly parsed, or file doesn't exist yet on the first copy.
I am having problem to properly understand this.
When you advise: Errors other than ENOENT should result in a logged message even on copy operation,
does it mean always log or only if debug mode or verbose mode is enabled?
There was a problem hiding this comment.
@marmarek I applied all the other requested changes. For the error handling & messaging, there are 5 cases within load_clipboard_metadata and two cases within get_clipboard_xevent_timestamp. I have passed a boolean variable to those function (g->log_level > 1). Maybe it is better to pass it as an integer and log based on different logging levels.
Better parsing and error handling for `/run/qubes/qubes-clipboard.bin.metadata` Fixes requested during review
alimirjamali
force-pushed
the
issue-9296
branch
from
e1e6026
to
b9aed51
Compare
|
According to the tests the copy operation for 64000 and more fails (with default config and increased limit too). I haven't found any error messages in logs (the metadata has copy_action=1, successful=1, cleared=1). |
|
Hmm, in some cases the 64k case works. But in none of the 4 iterations 200k worked. |
Was the fix I pushed to |
Looking at the comment from the bot: ...yes |
|
Ok. In the updated unittest in this line, it sets |
|
In test_002_clipboard_300k, does it expect a truncated zero byte I did the above as a temporary solution to INCR. Designing a signal from vmside to xside for the INCR is possible. |
|
For |
It intends to set the per-vm one (for the source VM specifically).
Ok, so the 300k test needs an update.
Right, the truncated one indeed should expect empty source. But I'm more worried about non-truncated one failing. |
I will look at test result again and see why it is failing. In the meantime, I am working with Marmarta on issue 9029 and will come back to this one very soon. Just for a change (classic c programming could grind mind) |
|
It could be also a race condition in the test. I've added sleep(1) after sending ctrl+shift+c for now, lets see if that changes anything. |
If this fails again, I appreciate if I could have the entire metadata. Is it possible? I am specifically interested on |
1st part of fix. Making the clipboard buffer size configurable via
max_clipboard_sizesetting withinguid.conffixes: QubesOS/qubes-issues#9296