Code review comment for lp:~kvalo/indicator-network/libconnman-service-properties

I didn't spot any errors as such, but there are some sections that can be simplified a great deal so marking as needsfixing. But really good work I must say.

313 +void connman_service_set_passphrase(ConnmanService *self,
314 + const gchar *passphrase)
315 +{
316 + ConnmanServicePrivate *priv = GET_PRIVATE(self);
317 + GValue value = {0};
318 +
319 + g_return_if_fail(CONNMAN_IS_SERVICE(self));
320 + g_return_if_fail(priv != NULL);
321 +
322 + g_value_init(&value, G_TYPE_STRING);
323 + g_value_set_string(&value, passphrase);
324 +
325 + g_object_set_property(G_OBJECT(self), "passphrase", &value);
326 +
327 + g_value_unset(&value);
328 +}

Considering all places you apply this pattern - you can do this a lot easier in 1 of 2 ways:

1)
  const gchar *newval;
  priv->stringprop = g_strup (newval);
  g_object_notify (self, "stringprop");

2)
  const gchar *newval;
  g_object_set (self, "stringprop", newval, NULL);

580 +void connman_service_set_domains_configuration(ConnmanService *self,
581 + gchar **domains)
582 +{
583 + ConnmanServicePrivate *priv = GET_PRIVATE(self);
584 + GValue value = {0};
585 + gchar **array;
586 + guint i, len;
587 +
588 + g_return_if_fail(CONNMAN_IS_SERVICE(self));
589 + g_return_if_fail(priv != NULL);
590 +
591 + /* FIXME: figure out and _document_ how to clear the domains */
592 + if (domains == NULL)
593 + return;
594 +
595 + len = g_strv_length(domains);
596 +
597 + array = g_new0(gchar *, len + 1);
598 +
599 + for (i = 0; i < len; i++)
600 + array[i] = g_strdup(domains[i]);
601 +
602 + array[len] = NULL;
603 +
604 + g_value_init(&value, G_TYPE_STRV);
605 + g_value_take_boxed(&value, array);
606 +
607 + g_object_set_property(G_OBJECT(self), "domains-configuration", &value);
608 +
609 + g_value_unset(&value);
610 +}

Why not use g_strdupv()? Or just g_value_set_boxed(&value, domains); Or even simpler g_object_set (self, "domains-configuration", domains, NULL); ? ;-) (this works because you have registered a boxed pspec of G_TYPE_STRV)

And some C nitpickery... The 'gchar**' you pass around are really 'const gchar* const*' - not saying you have to use this signature, but I personally like being anal with that :-)

811 +static void nameservers_updated(ConnmanService *self, GVariant *variant)
812 +{
813 + ConnmanServicePrivate *priv = GET_PRIVATE(self);
814 + GVariantIter iter;
815 + gchar **array, *server;
816 + guint i;
817 +
818 + g_strfreev(priv->nameservers);
819 +
820 + g_variant_iter_init(&iter, variant);
821 + i = 0;
822 + array = g_new0(char *, MAX_NAMESERVERS);
823 +
824 + while (g_variant_iter_next(&iter, "s", &server)) {
825 + array[i] = server;
826 + i++;
827 + if (i >= MAX_NAMESERVERS) {
828 + g_warning("libconnman: nameserver limit reached!");
829 + break;
830 + }
831 + }
832 +
833 + if (i == 0) {
834 + g_free(array);
835 + array = NULL;
836 + }
837 +
838 + priv->nameservers = array;
839 + g_object_notify(G_OBJECT(self), "nameservers");
840 +
841 + g_variant_unref(variant);
842 +}

g_variant_get_strv()?

And no need to prefix the g_warning() if you define a log domian on compile time. Something like -DG_LOG_DOMAIN=\"libconnman\"

1548 +static string list2str(string[] list) {
1549 + var result = "";
1550 + foreach (var s in list) {
1551 + result += s + " ";
1552 + }
1553 +
1554 + return result;
1555 +}

You can do:
  string.joinv(" ", list);
or
  string.join(" ", "elt1", "elt2", "elt3", ...);