Bug 212524 – [GTK] Bubblewrap sandbox lacks support for sndio

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Haelwenn (lanodan) Monnier]

Loosely related to https://bugs.webkit.org/show_bug.cgi?id=210101 (I'm using sndio as a workaround to this alsa bug)

[Haelwenn (lanodan) Monnier]

Created attachment 400580 [details]
Patch

[Carlos Alberto Lopez Perez]

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"

[Haelwenn (lanodan) Monnier]

(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.

[Adrian Perez]

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.

[Carlos Alberto Lopez Perez]

(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"

[Haelwenn (lanodan) Monnier]

(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.

[Haelwenn (lanodan) Monnier]

Created attachment 400667 [details]
Patch

[Adrian Perez]

(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 :)

[Haelwenn (lanodan) Monnier]

Any updates on merging it?

[Adrian Perez]

Comment on attachment 400667 [details]
Patch

Let's land this, patch LGTM now. Thanks for working on the patch!

[EWS]

Committed r263966: <https://trac.webkit.org/changeset/263966>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400667 [details].