> On January 12, 2016 at 5:00 PM Markus Armbruster <arm...@redhat.com> wrote: > separator = strchr(keys, '-'); > keyname_len = separator ? separator - keys : strlen(keys); > pstrcpy(keyname_buf, sizeof(keyname_buf), keys); > [...] > keyname_buf[keyname_len] = 0; > > This is stupid. If we already know how many characters we need, we > should copy exactly those:
I mentioned that and didn't touch it because the same holds for the copying of the word "less" below and should IMO be in a separate cleanup patch together with that one. > separator = strchr(keys, '-'); > keyname_len = separator ? separator - keys : strlen(keys); > if (keyname_len >= sizeof(keyname_buf)) > goto err_out; > memcpy(keyname_buf, keyname_len, keys); > keyname_buf[keyname_len] = 0; > > But I'd simply dispense with the static buffer, and do something like > > separator = strchr(keys, '-'); > key = separator ? g_strndup(keys, separator - keys) : g_strdup(keys); > [...] > g_free(key); > > This is advice, not recommendation. Sure, also a good alternative. > (...) > > Will you prepare a revised patch? Can do that tomorrow, but which option is the preferred one? If "%.*s" works everywhere then changing index_from_key() and using "%.*s" would be the most optimal I think. I don't want to bounce 5 more versions back and forth of something that's supposed to be rather trivial.
