On Sat, 2013-03-16 at 03:55 +0400, Vsevolod Velichko wrote:Thanks! I think it can actually be simplified somewhat, see below.
> Hello all,
>
> I'm widely using OpenVPN and recently found that network-manager-openvpn
> plugin just ignores all but first search domains it has to send to NM.
>
> I wrote a little patch, that obviously fixes the issue, however I'm not
> familiar with the libglib API at all, so it would be nice if someone check
> whether I put all frees correctly or not (when I wrote it, it wasn't
> obvious to me, how to deal with string arrays in glib and who controls each
> item at the moment).
For the openvpn parts, just allocate the GPtrArray before you start
> The second tiny patch is the patch for network-manager itself, it just
> fixes the logging of search domains: while they are received correctly,
> only the first one was logged.
putting stuff into it, eg right after:
/* DNS and WINS servers */
but before the for() loop. Pre-allocate some space in the array using
g_ptr_array_sized_new (3) or some low-but-not-too-low number. Then you
can simply do:
else if (g_str_has_prefix (tmp, "DOMAIN ") && !dns_domain)
g_ptr_array_add (dns_domains, tmp + 7);
of course, probably good to also check that strlen (tmp + 7) > 0 too.
Then, where the domains actually get added to the hash that gets sent to
NetworkManager, I think we can be clever here. Do you mind confirming
that for me? I think we can actually pass the GPtrArray directly
through without creating the char** and copying the elements. This
involves two things:
1) creating the dbus-glib "collection" that describes how to marshal a
GPtrArray with char* elements into a dbus array-of-string. To do this,
you define a new type near the top, see DBUS_TYPE_G_ARRAY_OF_UINT. So
it would be something like:
#define DBUS_TYPE_G_PTR_ARRAY_OF_STRING (dbus_g_type_get_collection ("GPtrArray", G_TYPE_STRING))
and then you can use this in #2.
2) creating a GValue that can hold the GPtrArray of strings
That type we just defined simply tells the GValue how to allocate and
free the pointer the GValue holds. Thus when you call g_value_unset()
(which the hash table you're inserting the GValue into does for you
automatically) the GValue knows that the pointer it holds is a GPtrArray
and that each element of the array is a char*. Thus it can free the
entire thing correctly. So down below a bit, you'd do something like
this:
if (dns_domains->len) {
val = g_slice_new0 (GValue);
g_value_init (val, DBUS_TYPE_G_PTR_ARRAY_OF_STRING);
g_value_take_boxed (val, dns_domains);
g_hash_table_insert (config, NM_VPN_PLUGIN_IP4_CONFIG_DOMAINS, val);
}
and that's it. The GPtrArray you've created is "taken over" by the
GValue and automatically freed when the GValue is freed (because we
called g_value_take_boxed() instead of g_value_set_boxed()).
Like I said, I think this should work, but if you can test it for me
that would be great. It should make the code a bit less complicated
there. If this code causes the plugin to crash, or NetworkManager to
reject the domains, then we've gotten something wrong and perhaps we
make this change. But we'll see.
Thanks!
Dan
Attachment:
0001-send-all-search-domains-to-nm.patch
Description: Binary data