Summary: | [GTK] Bubblewrap sandbox lacks support for sndio | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Haelwenn (lanodan) Monnier <contact+bugs.webkit.org> | ||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aperez, bugs-noreply, clopez | ||||||
Priority: | P2 | ||||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Haelwenn (lanodan) Monnier
2020-05-29 07:50:47 PDT
Created attachment 400580 [details]
Patch
Comment on attachment 400580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400580&action=review > Source/WebKit/ChangeLog:4 > + BubblewrapLauncher.cpp: Add paths required for sndio > + You have to add the bug number here. Check how other Changelog entries are written. > Source/WebKit/ChangeLog:11 > + No new tests (OOPS!). You have to remove this line and change it by something like "Covered by existing tests" (In reply to Carlos Alberto Lopez Perez from comment #2) > Comment on attachment 400580 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400580&action=review > > Source/WebKit/ChangeLog:11 > > + No new tests (OOPS!). > > You have to remove this line and change it by something like "Covered by > existing tests" Well there *is* no tests for sndio into the webkit bubblewrap sandbox so I'm not really sure what to write there. Comment on attachment 400580 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=400580&action=review > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:386 > + bindIfExists(args, "/tmp/sndio", BindFlags::ReadWrite); Does the patch work without this bind mount? I think it should be enought to bind the per-user directory (as done in the next two lines). I tried to run “sndiod” in my system and there is only a “/tmp/sndio-1000” directory (my user UID here is 1000), but no “/tmp/sndio” directory. (In reply to Haelwenn (lanodan) Monnier from comment #3) > (In reply to Carlos Alberto Lopez Perez from comment #2) > > Comment on attachment 400580 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=400580&action=review > > > Source/WebKit/ChangeLog:11 > > > + No new tests (OOPS!). > > > > You have to remove this line and change it by something like "Covered by > > existing tests" > > Well there *is* no tests for sndio into the webkit bubblewrap sandbox so I'm > not really sure what to write there. I know, but you have to remove that line "No new tests (OOPS!)." to make our tooling happy. So saying something along with what I suggested is good enough. You can also say something like "Feature has been manually tested, there is currently not automatic tests for it" (In reply to Adrian Perez from comment #4) > Comment on attachment 400580 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=400580&action=review > > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:386 > > + bindIfExists(args, "/tmp/sndio", BindFlags::ReadWrite); > > Does the patch work without this bind mount? I think it should be enought to > bind the per-user directory (as done in the next two lines). I tried to run > “sndiod” in my system and there is only a “/tmp/sndio-1000” directory (my > user UID here is 1000), but no “/tmp/sndio” directory. Yes, the "/tmp/sndio" is needed when sndio is launched as superuser (which then drops to an unprivileged user like "sndiod"), this allows to share it between multiple users. Created attachment 400667 [details]
Patch
(In reply to Haelwenn (lanodan) Monnier from comment #6) > (In reply to Adrian Perez from comment #4) > > Comment on attachment 400580 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=400580&action=review > > > > > Source/WebKit/UIProcess/Launcher/glib/BubblewrapLauncher.cpp:386 > > > + bindIfExists(args, "/tmp/sndio", BindFlags::ReadWrite); > > > > Does the patch work without this bind mount? I think it should be enought to > > bind the per-user directory (as done in the next two lines). I tried to run > > “sndiod” in my system and there is only a “/tmp/sndio-1000” directory (my > > user UID here is 1000), but no “/tmp/sndio” directory. > > Yes, the "/tmp/sndio" is needed when sndio is launched as superuser (which > then drops to an unprivileged user like "sndiod"), this allows to share it > between multiple users. Ah, good to know, thanks for commenting about this :) Any updates on merging it? Comment on attachment 400667 [details]
Patch
Let's land this, patch LGTM now. Thanks for working on the patch!
Committed r263966: <https://trac.webkit.org/changeset/263966> All reviewed patches have been landed. Closing bug and clearing flags on attachment 400667 [details]. |