Re: [systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-23 Thread Jan Synacek 
Lennart Poettering lenn...@poettering.net writes:

 On Mon, 20.10.14 12:43, Jan Synacek (jsyna...@redhat.com) wrote:

 When setting any of those using set-x11-keymap, check that their values
 are available on the system.
 ---
  src/locale/localectl.c | 208 
 +
  1 file changed, 139 insertions(+), 69 deletions(-)
 
 diff --git a/src/locale/localectl.c b/src/locale/localectl.c
 index 3690f9f..0caf467 100644
 --- a/src/locale/localectl.c
 +++ b/src/locale/localectl.c
 @@ -53,6 +53,14 @@ static BusTransport arg_transport = BUS_TRANSPORT_LOCAL;
  static char *arg_host = NULL;
  static bool arg_convert = true;
  
 +enum keymap_state {
 +NONE,
 +MODELS   = 1  0,
 +LAYOUTS  = 1  1,
 +VARIANTS = 1  2,
 +OPTIONS  = 1  3
 +};

 Hmm, why is this a bitfield? Do I get this patch right and you are
 trying to match the passed arguments to all
 models/layouts/variants/options all the time? This means if a layout
 happens to have the same name as a model then things will be
 ambiguous? I really don't like this I must say.

Basic idea was that get_x11_keymaps_internal() gets a flag that says
what kind of information should be parsed and returned from
/usr/share/X11/xkb/rules/base.lst. If only a layout needs to be
specified, then fetch just layouts. If a layout, model and options all
get specified, fetch all of them in one go. If I left the function as it
was, it would require multiple passes to validate layouts, models and
options together.

And yes, there's ambiguity in that solution. I didn't come up with
anything better, I'm not even sure if this has a proper solution without
using X libraries, which is IMO unthinkable in this case. If you have a
better idea, I'll be happy to rework this solution with something
better.

 Or mabye I am not following what you are doing here? Can you elaborate?

 Also, this validating really be done on the server side, not
 on the client side, to take full effect. 

 Doing the listing/completion thing on the client-side appears OK since
 it's really more a help to the user, nothing that validates stuff. But
 if we want to validate input here, we should really do it on the
 server-side, in localed, especially as localed and localectl might run
 on different systems and not share the same map list.

Doing this on the server side makes sense. It didn't occur to me
before. I saw code that already parsed the local xkb rules, so I just
improved on that.

Also, in case of a failed validation, it's probably better to just warn
the user and go on, as mentioned earlier in this thread.

So, if would it make sense to implement the server-side validation
functionality? Would the DBus API need to be changed? Any other ideas?

 Lennart

Cheers,
-- 
Jan Synacek
Software Engineer, Red Hat


signature.asc
Description: PGP signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-23 Thread Lennart Poettering 
On Thu, 23.10.14 16:22, Jan Synacek (jsyna...@redhat.com) wrote:

  Hmm, why is this a bitfield? Do I get this patch right and you are
  trying to match the passed arguments to all
  models/layouts/variants/options all the time? This means if a layout
  happens to have the same name as a model then things will be
  ambiguous? I really don't like this I must say.
 
 Basic idea was that get_x11_keymaps_internal() gets a flag that says
 what kind of information should be parsed and returned from
 /usr/share/X11/xkb/rules/base.lst. If only a layout needs to be
 specified, then fetch just layouts. If a layout, model and options all
 get specified, fetch all of them in one go. If I left the function as it
 was, it would require multiple passes to validate layouts, models and
 options together.

Well, one option would be to make the function take all four items and
check them all in one go.

  Doing the listing/completion thing on the client-side appears OK since
  it's really more a help to the user, nothing that validates stuff. But
  if we want to validate input here, we should really do it on the
  server-side, in localed, especially as localed and localectl might run
  on different systems and not share the same map list.
 
 Doing this on the server side makes sense. It didn't occur to me
 before. I saw code that already parsed the local xkb rules, so I just
 improved on that.
 
 Also, in case of a failed validation, it's probably better to just warn
 the user and go on, as mentioned earlier in this thread.
 
 So, if would it make sense to implement the server-side validation
 functionality? Would the DBus API need to be changed? Any other ideas?

I think we should just move the parsing/validation of the maps file
from localectl into some file src/share/xkb-util.[ch] or so, and then
use it for listing things on the client side, and for validation on
the server side.

Something similar we already did for the locales stuff which we moved
to src/share/locale-util.[ch] (though that was arguably a simpler
case, since returning a simple strv for the locales works well...)

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-20 Thread Jan Synacek 
Ran Benita ran...@gmail.com writes:
 On Fri, Oct 17, 2014 at 02:02:13PM +0200, Jan Synacek wrote:
 When setting any of those using set-x11-keymap, check that their values
 are available on the system.

 I have only skimmed this patch, but generally:

 1. There can only be one model.
 2. There can be multiple layouts and variants, comma separated. E.g.,
layout=us,il variant=,lyx. The amount of layouts and variants should
match (or the variants can be empty), but most stuff you throw at it
will be accepted though.
 3. There can be multiple comma-separated options.

 Do you handle input like layout=us,il variant=,lyx correctly?

 Ran

Nope, I didn't realize that. I'll send a better version of the patch.

The parsing won't be perfect, though. With setxkbmap, I can do the
following without any error:

# setxkbmap us,cz lyx ctrl:nocaps -model idontexist
# echo $?
0

Note that there is no lyx variant for either us or cz. Also, the number
of layouts and variants *doesn't* match and the model doesn't exist as
well. (That's probably what you meant by the last sentence in 2., I'm
just restating it to be clear).

Cheers,
-- 
Jan Synacek
Software Engineer, Red Hat


signature.asc
Description: PGP signature
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-20 Thread Jan Synacek 
When setting any of those using set-x11-keymap, check that their values
are available on the system.
---
 src/locale/localectl.c | 208 +
 1 file changed, 139 insertions(+), 69 deletions(-)

diff --git a/src/locale/localectl.c b/src/locale/localectl.c
index 3690f9f..0caf467 100644
--- a/src/locale/localectl.c
+++ b/src/locale/localectl.c
@@ -53,6 +53,14 @@ static BusTransport arg_transport = BUS_TRANSPORT_LOCAL;
 static char *arg_host = NULL;
 static bool arg_convert = true;
 
+enum keymap_state {
+NONE,
+MODELS   = 1  0,
+LAYOUTS  = 1  1,
+VARIANTS = 1  2,
+OPTIONS  = 1  3
+};
+
 static void pager_open_if_enabled(void) {
 
 if (arg_no_pager)
@@ -350,59 +358,12 @@ static int list_vconsole_keymaps(sd_bus *bus, char 
**args, unsigned n) {
 return 0;
 }
 
-static int set_x11_keymap(sd_bus *bus, char **args, unsigned n) {
-_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
-const char *layout, *model, *variant, *options;
-int r;
-
-assert(bus);
-assert(args);
-
-if (n  5) {
-log_error(Too many arguments.);
-return -EINVAL;
-}
-
-polkit_agent_open_if_enabled();
-
-layout = args[1];
-model = n  2 ? args[2] : ;
-variant = n  3 ? args[3] : ;
-options = n  4 ? args[4] : ;
-
-r = sd_bus_call_method(
-bus,
-org.freedesktop.locale1,
-/org/freedesktop/locale1,
-org.freedesktop.locale1,
-SetX11Keyboard,
-error,
-NULL,
-bb, layout, model, variant, options,
-  arg_convert, arg_ask_password);
-if (r  0)
-log_error(Failed to set keymap: %s, 
bus_error_message(error, -r));
-
-return r;
-}
-
-static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) {
+static int get_x11_keymaps_internal(char ***list, enum keymap_state look_for, 
const char *layout)
+{
 _cleanup_fclose_ FILE *f = NULL;
-_cleanup_strv_free_ char **list = NULL;
 char line[LINE_MAX];
-enum {
-NONE,
-MODELS,
-LAYOUTS,
-VARIANTS,
-OPTIONS
-} state = NONE, look_for;
-int r;
-
-if (n  2) {
-log_error(Too many arguments.);
-return -EINVAL;
-}
+enum keymap_state state = NONE;
+int r = 0;
 
 f = fopen(/usr/share/X11/xkb/rules/base.lst, re);
 if (!f) {
@@ -410,17 +371,6 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
unsigned n) {
 return -errno;
 }
 
-if (streq(args[0], list-x11-keymap-models))
-look_for = MODELS;
-else if (streq(args[0], list-x11-keymap-layouts))
-look_for = LAYOUTS;
-else if (streq(args[0], list-x11-keymap-variants))
-look_for = VARIANTS;
-else if (streq(args[0], list-x11-keymap-options))
-look_for = OPTIONS;
-else
-assert_not_reached(Wrong parameter);
-
 FOREACH_LINE(line, f, break) {
 char *l, *w;
 
@@ -444,12 +394,12 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
unsigned n) {
 continue;
 }
 
-if (state != look_for)
+if (!(state  look_for))
 continue;
 
 w = l + strcspn(l, WHITESPACE);
 
-if (n  1) {
+if (layout) {
 char *e;
 
 if (*w == 0)
@@ -465,23 +415,143 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
unsigned n) {
 
 *e = 0;
 
-if (!streq(w, args[1]))
+if (!streq(w, layout))
 continue;
 } else
 *w = 0;
 
- r = strv_extend(list, l);
+ r = strv_extend(list, l);
  if (r  0)
  return log_oom();
 }
 
-if (strv_isempty(list)) {
+if (strv_isempty(*list)) {
 log_error(Couldn't find any entries.);
 return -ENOENT;
 }
 
-strv_sort(list);
-strv_uniq(list);
+strv_sort(*list);
+strv_uniq(*list);
+
+return r;
+}
+
+static int set_x11_keymap(sd_bus *bus, char **args, unsigned n) {
+_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
+_cleanup_strv_free_ char **list = NULL, **layouts = NULL, **variants = 
NULL, **options = NULL;
+char **it;
+const char *layouts_arg, *variants_arg, *options_arg;
+

Re: [systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-20 Thread Lennart Poettering 
On Mon, 20.10.14 12:43, Jan Synacek (jsyna...@redhat.com) wrote:

 When setting any of those using set-x11-keymap, check that their values
 are available on the system.
 ---
  src/locale/localectl.c | 208 
 +
  1 file changed, 139 insertions(+), 69 deletions(-)
 
 diff --git a/src/locale/localectl.c b/src/locale/localectl.c
 index 3690f9f..0caf467 100644
 --- a/src/locale/localectl.c
 +++ b/src/locale/localectl.c
 @@ -53,6 +53,14 @@ static BusTransport arg_transport = BUS_TRANSPORT_LOCAL;
  static char *arg_host = NULL;
  static bool arg_convert = true;
  
 +enum keymap_state {
 +NONE,
 +MODELS   = 1  0,
 +LAYOUTS  = 1  1,
 +VARIANTS = 1  2,
 +OPTIONS  = 1  3
 +};

Hmm, why is this a bitfield? Do I get this patch right and you are
trying to match the passed arguments to all
models/layouts/variants/options all the time? This means if a layout
happens to have the same name as a model then things will be
ambiguous? I really don't like this I must say.

Or mabye I am not following what you are doing here? Can you elaborate?

Also, this validating really be done on the server side, not
on the client side, to take full effect. 

Doing the listing/completion thing on the client-side appears OK since
it's really more a help to the user, nothing that validates stuff. But
if we want to validate input here, we should really do it on the
server-side, in localed, especially as localed and localectl might run
on different systems and not share the same map list.

Lennart

-- 
Lennart Poettering, Red Hat
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-20 Thread Ran Benita 
On Mon, Oct 20, 2014 at 11:15:14AM +0200, Jan Synacek wrote:
 Ran Benita ran...@gmail.com writes:
  On Fri, Oct 17, 2014 at 02:02:13PM +0200, Jan Synacek wrote:
  When setting any of those using set-x11-keymap, check that their values
  are available on the system.
 
  I have only skimmed this patch, but generally:
 
  1. There can only be one model.
  2. There can be multiple layouts and variants, comma separated. E.g.,
 layout=us,il variant=,lyx. The amount of layouts and variants should
 match (or the variants can be empty), but most stuff you throw at it
 will be accepted though.
  3. There can be multiple comma-separated options.
 
  Do you handle input like layout=us,il variant=,lyx correctly?
 
  Ran
 
 Nope, I didn't realize that. I'll send a better version of the patch.
 
 The parsing won't be perfect, though. With setxkbmap, I can do the
 following without any error:
 
 # setxkbmap us,cz lyx ctrl:nocaps -model idontexist
 # echo $?
 0
 
 Note that there is no lyx variant for either us or cz. Also, the number
 of layouts and variants *doesn't* match and the model doesn't exist as
 well. (That's probably what you meant by the last sentence in 2., I'm
 just restating it to be clear).

Right, these settings are pretty permissive.

But you reminded me - it is probably not a good idea to entirely abort
the operation if the provided arguments don't validate. That's because
the XKB rules file (/usr/share/X11/xkb/rules/) support wildcards, as you
noticed with the idontexist model. These are then passed along, and some
of them are in fact valid as far as I remember, just not mentioned in the
base.lst file.

So I think this should be a warning, not an error. If the user knows
what he intended, he can ignore the error. Otherwise he will correct the
error. Not ideal, but better than preventing legal values.
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-17 Thread Jan Synacek 
When setting any of those using set-x11-keymap, check that their values
are available on the system.
---
 src/locale/localectl.c | 179 ++---
 1 file changed, 110 insertions(+), 69 deletions(-)

diff --git a/src/locale/localectl.c b/src/locale/localectl.c
index 3690f9f..79bc2d0 100644
--- a/src/locale/localectl.c
+++ b/src/locale/localectl.c
@@ -53,6 +53,14 @@ static BusTransport arg_transport = BUS_TRANSPORT_LOCAL;
 static char *arg_host = NULL;
 static bool arg_convert = true;
 
+enum keymap_state {
+NONE,
+MODELS   = 1  0,
+LAYOUTS  = 1  1,
+VARIANTS = 1  2,
+OPTIONS  = 1  3
+};
+
 static void pager_open_if_enabled(void) {
 
 if (arg_no_pager)
@@ -350,59 +358,12 @@ static int list_vconsole_keymaps(sd_bus *bus, char 
**args, unsigned n) {
 return 0;
 }
 
-static int set_x11_keymap(sd_bus *bus, char **args, unsigned n) {
-_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
-const char *layout, *model, *variant, *options;
-int r;
-
-assert(bus);
-assert(args);
-
-if (n  5) {
-log_error(Too many arguments.);
-return -EINVAL;
-}
-
-polkit_agent_open_if_enabled();
-
-layout = args[1];
-model = n  2 ? args[2] : ;
-variant = n  3 ? args[3] : ;
-options = n  4 ? args[4] : ;
-
-r = sd_bus_call_method(
-bus,
-org.freedesktop.locale1,
-/org/freedesktop/locale1,
-org.freedesktop.locale1,
-SetX11Keyboard,
-error,
-NULL,
-bb, layout, model, variant, options,
-  arg_convert, arg_ask_password);
-if (r  0)
-log_error(Failed to set keymap: %s, 
bus_error_message(error, -r));
-
-return r;
-}
-
-static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) {
+static int get_x11_keymaps_internal(char ***list, enum keymap_state look_for, 
const char *layout)
+{
 _cleanup_fclose_ FILE *f = NULL;
-_cleanup_strv_free_ char **list = NULL;
 char line[LINE_MAX];
-enum {
-NONE,
-MODELS,
-LAYOUTS,
-VARIANTS,
-OPTIONS
-} state = NONE, look_for;
-int r;
-
-if (n  2) {
-log_error(Too many arguments.);
-return -EINVAL;
-}
+enum keymap_state state = NONE;
+int r = 0;
 
 f = fopen(/usr/share/X11/xkb/rules/base.lst, re);
 if (!f) {
@@ -410,17 +371,6 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
unsigned n) {
 return -errno;
 }
 
-if (streq(args[0], list-x11-keymap-models))
-look_for = MODELS;
-else if (streq(args[0], list-x11-keymap-layouts))
-look_for = LAYOUTS;
-else if (streq(args[0], list-x11-keymap-variants))
-look_for = VARIANTS;
-else if (streq(args[0], list-x11-keymap-options))
-look_for = OPTIONS;
-else
-assert_not_reached(Wrong parameter);
-
 FOREACH_LINE(line, f, break) {
 char *l, *w;
 
@@ -444,12 +394,12 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
unsigned n) {
 continue;
 }
 
-if (state != look_for)
+if (!(state  look_for))
 continue;
 
 w = l + strcspn(l, WHITESPACE);
 
-if (n  1) {
+if (layout) {
 char *e;
 
 if (*w == 0)
@@ -465,23 +415,114 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
unsigned n) {
 
 *e = 0;
 
-if (!streq(w, args[1]))
+if (!streq(w, layout))
 continue;
 } else
 *w = 0;
 
- r = strv_extend(list, l);
+ r = strv_extend(list, l);
  if (r  0)
  return log_oom();
 }
 
-if (strv_isempty(list)) {
+if (strv_isempty(*list)) {
 log_error(Couldn't find any entries.);
 return -ENOENT;
 }
 
-strv_sort(list);
-strv_uniq(list);
+strv_sort(*list);
+strv_uniq(*list);
+
+return r;
+}
+
+static int set_x11_keymap(sd_bus *bus, char **args, unsigned n) {
+_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
+_cleanup_strv_free_ char **list = NULL;
+const char *layout, *model, *variant, *options;
+enum keymap_state look_for;
+int r;
+
+assert(bus);
+

Re: [systemd-devel] [PATCH 2/2] localectl: verify layout, model, variant and options

2014-10-17 Thread Ran Benita 
On Fri, Oct 17, 2014 at 02:02:13PM +0200, Jan Synacek wrote:
 When setting any of those using set-x11-keymap, check that their values
 are available on the system.

I have only skimmed this patch, but generally:

1. There can only be one model.
2. There can be multiple layouts and variants, comma separated. E.g.,
   layout=us,il variant=,lyx. The amount of layouts and variants should
   match (or the variants can be empty), but most stuff you throw at it
   will be accepted though.
3. There can be multiple comma-separated options.

Do you handle input like layout=us,il variant=,lyx correctly?

Ran

 ---
  src/locale/localectl.c | 179 
 ++---
  1 file changed, 110 insertions(+), 69 deletions(-)
 
 diff --git a/src/locale/localectl.c b/src/locale/localectl.c
 index 3690f9f..79bc2d0 100644
 --- a/src/locale/localectl.c
 +++ b/src/locale/localectl.c
 @@ -53,6 +53,14 @@ static BusTransport arg_transport = BUS_TRANSPORT_LOCAL;
  static char *arg_host = NULL;
  static bool arg_convert = true;
  
 +enum keymap_state {
 +NONE,
 +MODELS   = 1  0,
 +LAYOUTS  = 1  1,
 +VARIANTS = 1  2,
 +OPTIONS  = 1  3
 +};
 +
  static void pager_open_if_enabled(void) {
  
  if (arg_no_pager)
 @@ -350,59 +358,12 @@ static int list_vconsole_keymaps(sd_bus *bus, char 
 **args, unsigned n) {
  return 0;
  }
  
 -static int set_x11_keymap(sd_bus *bus, char **args, unsigned n) {
 -_cleanup_bus_error_free_ sd_bus_error error = SD_BUS_ERROR_NULL;
 -const char *layout, *model, *variant, *options;
 -int r;
 -
 -assert(bus);
 -assert(args);
 -
 -if (n  5) {
 -log_error(Too many arguments.);
 -return -EINVAL;
 -}
 -
 -polkit_agent_open_if_enabled();
 -
 -layout = args[1];
 -model = n  2 ? args[2] : ;
 -variant = n  3 ? args[3] : ;
 -options = n  4 ? args[4] : ;
 -
 -r = sd_bus_call_method(
 -bus,
 -org.freedesktop.locale1,
 -/org/freedesktop/locale1,
 -org.freedesktop.locale1,
 -SetX11Keyboard,
 -error,
 -NULL,
 -bb, layout, model, variant, options,
 -  arg_convert, arg_ask_password);
 -if (r  0)
 -log_error(Failed to set keymap: %s, 
 bus_error_message(error, -r));
 -
 -return r;
 -}
 -
 -static int list_x11_keymaps(sd_bus *bus, char **args, unsigned n) {
 +static int get_x11_keymaps_internal(char ***list, enum keymap_state 
 look_for, const char *layout)
 +{
  _cleanup_fclose_ FILE *f = NULL;
 -_cleanup_strv_free_ char **list = NULL;
  char line[LINE_MAX];
 -enum {
 -NONE,
 -MODELS,
 -LAYOUTS,
 -VARIANTS,
 -OPTIONS
 -} state = NONE, look_for;
 -int r;
 -
 -if (n  2) {
 -log_error(Too many arguments.);
 -return -EINVAL;
 -}
 +enum keymap_state state = NONE;
 +int r = 0;
  
  f = fopen(/usr/share/X11/xkb/rules/base.lst, re);
  if (!f) {
 @@ -410,17 +371,6 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
 unsigned n) {
  return -errno;
  }
  
 -if (streq(args[0], list-x11-keymap-models))
 -look_for = MODELS;
 -else if (streq(args[0], list-x11-keymap-layouts))
 -look_for = LAYOUTS;
 -else if (streq(args[0], list-x11-keymap-variants))
 -look_for = VARIANTS;
 -else if (streq(args[0], list-x11-keymap-options))
 -look_for = OPTIONS;
 -else
 -assert_not_reached(Wrong parameter);
 -
  FOREACH_LINE(line, f, break) {
  char *l, *w;
  
 @@ -444,12 +394,12 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
 unsigned n) {
  continue;
  }
  
 -if (state != look_for)
 +if (!(state  look_for))
  continue;
  
  w = l + strcspn(l, WHITESPACE);
  
 -if (n  1) {
 +if (layout) {
  char *e;
  
  if (*w == 0)
 @@ -465,23 +415,114 @@ static int list_x11_keymaps(sd_bus *bus, char **args, 
 unsigned n) {
  
  *e = 0;
  
 -if (!streq(w, args[1]))
 +if (!streq(w, layout))
  continue;
  } else
  *w = 0;
  
 - r = strv_extend(list, l);
 + r = strv_extend(list, l);
   if (r  0)
   return log_oom();