GNOME.org
 

Re: [PATCH] Multiple search domains in OpenVPN

On Mon, 2013-03-18 at 21:01 +0400, Vsevolod Velichko wrote:

Thanks, Dan!

Your way is much simpler than one of mine.


Looks good; thanks!  Pushed this.  I added one piece because I forgot
about it in my earlier reply; we do need to free the GPtrArray when no
domains are received from OpenVPN, because in that case the ownership of
the array is not taken by the GValue.

Dan


I tested it and found it working, so I'm attaching a new patch.

Best wishes and have a nice day,
Vsevolod Velichko


2013/3/18 Dan Williams <dcbw redhat com>


On Sat, 2013-03-16 at 03:55 +0400, Vsevolod Velichko wrote:

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).


Thanks!  I think it can actually be simplified somewhat, see below.


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.


For the openvpn parts, just allocate the GPtrArray before you start
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
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]