[systemd-devel] [PATCH v2] localed: validate set-x11-keymap input

[Lennart Poettering]

On Mon, 24.11.14 13:31, Jan Synacek (jsynacek at redhat.com) wrote:

> Lennart Poettering <lennart at poettering.net> writes:
> > On Fri, 14.11.14 12:42, Jan Synacek (jsynacek at redhat.com) wrote:
> >> +        if (look_for == LAYOUTS) {
> >> +                Set *s;
> >> +                char *k;
> >> +                Iterator i;
> >> +                /* XXX: Is there a better way to sort Hashmap keys? */
> >> +                _cleanup_strv_free_ char **tmp = NULL;
> >> +
> >> +                HASHMAP_FOREACH_KEY(s, k, keymap->x11_layouts, i)
> >> +                        if (strv_extend(&tmp, k) < 0)
> >> +                                (void) log_oom();
> >
> > There's hashmap_get_strv() for cases like this.
> 
> I need keys, not values. I didn't find any hashmap_get_strv() equivalent
> for keys.

Maybe add it then? I think this might become useful even beyond the
kbd mapping part one day.

> > Also, please neer invoke strv_extend() when appending to unbounded
> > arrays. It's slow! 
> 
> What is a preffered way to extend a strv? In this case, I know the final
> size, so it the array could manually be copied. I don't think that
> that's very nice, though.

Best case: you know the precise size of the array in the end. In that
case, simply use realloc()/realloc_multiply() to allocate the right
sized array, fill it in, done.

If you don#t know the size, but you guess that it might be quite a few
entries in there in the end and you might be adding a good chunk of
them, please use greedy_realloc(). It grows the array exponentially,
which is very beneficial performance-wise.

If you don't know the size, but you are pretty sure that it will never
gro to more than 10 entries or so, and that you are just going to
add a couple of them, then strv_extend()/strv_push()/strv_consume()
are OK.

> > Humm, when clearing up hashmaps please just write a loop that steals
> > the first entry, free it and then repeat. Iterating through a hashmap
> > while deleting its entries is unnecessary...
> 
> I need to free its entries *and* keys. I didn't find any function that I
> could use for that, apart from hashmap_free_free(), which crashes in
> my case.

while ((k = hashmap_first_key())) {
      v = hashmap_steal_first();
      free(v);
      free(k);
}

Lennart

-- 
Lennart Poettering, Red Hat