[PATCH weston] screen-share: Allow fullscreen shell command to be configured
Andrew Wedgbury
andrew.wedgbury at realvnc.com
Fri May 2 01:45:23 PDT 2014
Hi Jason,
On Thu, 1 May 2014, Jason Ekstrand wrote:
> On Thu, May 1, 2014 at 9:00 AM, Andrew Wedgbury <andrew.wedgbury at realvnc.com> wrote:
> Currently the screen-share module uses a hard-coded command to start the
> fullscreen shell server. This patch causes the module to read the command from
> the weston config file (from the "command" key in the "screen-share" section).
> The default value remains the same (i.e. to run weston with the RDP backend and
> fullscreen shell), but is now located in the weston config file.
>
> As well as allowing the arguments to the fullscreen shell server to be changed,
> this also permits an alternative fullscreen shell server to be used if required,
> without needing to recompile. Since the command is run as the user running
> weston, this should not pose any additional security risk.
>
> ---
> src/screen-share.c | 62 ++++++++++++++++++++++++++++++++++++++++++------------
> weston.ini.in | 3 +++
> 2 files changed, 51 insertions(+), 14 deletions(-)
>
> diff --git a/src/screen-share.c b/src/screen-share.c
> index 6f60b81..6e9a14f 100644
> --- a/src/screen-share.c
> +++ b/src/screen-share.c
> @@ -32,6 +32,7 @@
> #include <signal.h>
> #include <linux/input.h>
> #include <errno.h>
> +#include <ctype.h>
>
> #include <wayland-client.h>
>
> @@ -101,6 +102,11 @@ struct ss_shm_buffer {
> pixman_image_t *pm_image;
> };
>
> +struct screen_share {
> + struct weston_compositor *compositor;
> + char *command;
> +};
> +
> static void
> ss_seat_handle_pointer_enter(void *data, struct wl_pointer *pointer,
> uint32_t serial, struct wl_surface *surface,
> @@ -982,13 +988,15 @@ shared_output_destroy(struct shared_output *so)
> }
>
> static struct shared_output *
> -weston_output_share(struct weston_output *output,
> - const char *path, char *const argv[])
> +weston_output_share(struct weston_output *output, const char* command)
> {
> int sv[2];
> char str[32];
> pid_t pid;
> sigset_t allsigs;
> + struct wl_array args;
> + char **argv;
> + char *start, *p, **ps;
>
> if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, sv) < 0) {
> weston_log("weston_output_share: socketpair failed: %m\n");
> @@ -1025,7 +1033,28 @@ weston_output_share(struct weston_output *output,
> snprintf(str, sizeof str, "%d", sv[1]);
> setenv("WAYLAND_SERVER_SOCKET", str, 1);
>
> - execv(path, argv);
> + wl_array_init(&args);
> + start = zalloc(strlen(command)+1);
> + if (start == 0)
> + return NULL;
> + strcpy(start, command);
> + while (*start) {
> + for (p = start; *p && !isspace(*p); p++)
> + ;
> +
> + ps = wl_array_add(&args, sizeof *ps);
> + *ps = start;
> +
> + while (*p && isspace(*p))
> + *p++ = '\0';
> +
> + start = p;
> + }
> + ps = wl_array_add(&args, sizeof *ps);
> + *ps = NULL;
> +
> + argv = args.data;
> + execv(argv[0], argv);
> weston_log("weston_output_share: exec failed: %m\n");
> abort();
> } else {
> @@ -1056,7 +1085,7 @@ share_output_binding(struct weston_seat *seat, uint32_t time, uint32_t key,
> void *data)
> {
> struct weston_output *output;
> - const char *path = BINDIR "/weston";
> + struct screen_share *ss = data;
>
> if (!seat->pointer) {
> weston_log("Cannot pick output: Seat does not have pointer\n");
> @@ -1071,23 +1100,28 @@ share_output_binding(struct weston_seat *seat, uint32_t time, uint32_t key,
> return;
> }
>
> - char *const argv[] = {
> - "weston",
> - "--backend=rdp-backend.so",
> - "--shell=fullscreen-shell.so",
> - "--no-clients-resize",
> - NULL
> - };
> -
> - weston_output_share(output, path, argv);
> + weston_output_share(output, ss->command);
>
>
> Why are we doing our own command parsing? Why not just run "sh -c command" instead of doing our own command-line parsing? Then we could just do:
>
> char *const argv[] = {
> "/bin/sh",
> "-c",
> command,
> NULL
> };
>
> It's much simpler and would allow for proper quoting etc. Is there a good reason why we shouldn't do this?
Yes, agreed it is much simpler and ultimately allows for more flexibility
with quoting and such. Really the only reason I did it like this is that I
was basing it on how launcher commands are handled in
clients/desktop-shell.c. I wonder if that should be changed too?
>
> }
>
> WL_EXPORT int
> module_init(struct weston_compositor *compositor,
> int *argc, char *argv[])
> {
> + struct screen_share *ss;
> + struct weston_config_section *section;
> +
> + ss = zalloc(sizeof *ss);
> + if (ss == NULL)
> + return -1;
> + ss->compositor = compositor;
> +
> + section = weston_config_get_section(compositor->config, "screen-share",
> + NULL, NULL);
> +
> + weston_config_section_get_string(section, "command", &ss->command, "");
>
>
> Do we want an actual default here rather than just putting something in the default config file?
My preference would be to have the default in the config file. It just
seems too specific a value to hard code here. Putting it in the config
file means that people can easily see what the default is, enabling them
to easily adjust the arguments if required.
>
> +
> weston_compositor_add_key_binding(compositor, KEY_S,
> MODIFIER_CTRL | MODIFIER_ALT,
> - share_output_binding, compositor);
> + share_output_binding, ss);
> return 0;
> }
> diff --git a/weston.ini.in b/weston.ini.in
> index 2c39177..1f216a7 100644
> --- a/weston.ini.in
> +++ b/weston.ini.in
> @@ -65,3 +65,6 @@ path=@libexecdir@/weston-keyboard
> #constant_accel_factor = 50
> #min_accel_factor = 0.16
> #max_accel_factor = 1.0
> +
> +[screen-share]
> +command=@bindir@/weston --backend=rdp-backend.so --shell=fullscreen-shell.so --no-clients-resize
> --
> 1.9.2
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
>
>
>
Thanks for your comments,
---
Andrew Wedgbury <andrew.wedgbury at realvnc.com>
More information about the wayland-devel
mailing list