Created attachment 177021 [details] [review]
Patch for glib which changes and adds test code

Created attachment 177022 [details] [review]
Patch to glib-networking which updates gnutls implementation

On IRC, Dan suggested just replacing this property so it returns the DER DNs. So here's a patch which does that.

One thing I'm not clear on is whether g_tls_client_connection_gnutls_retrieve_function() can be called multiple times per GTlsClientConnection object. If so then we need to free any current priv->accepted_cas value before assigning a new value to it.

(In reply to comment #3)
> One thing I'm not clear on is whether
> g_tls_client_connection_gnutls_retrieve_function() can be called multiple times
> per GTlsClientConnection object.

It can, if you rehandshake.

Created attachment 177025 [details] [review]
Second patch to glib-networking

Fixes the leak when accepted-cas is set multiple times.

Comment on attachment 177021 [details] [review]
Patch for glib which changes and adds test code

>This property is now a GPtrArray of GByteArray values. Each

For whatever reason, glib generally does not use GPtrArray in public APIs, Make it a GList.

You'll then need to introspection-annotate it... for _get_accepted_cas() it would be

 * Return value: (element-type GByteArray) (transfer full): ...

For the property definition it would be

 * Type: GList<GByteArray>
 * Transfer: full

>+                      g_printerr ("  Authority has %sDER DN of length: %d\n",
>+                                  dn->data[0] == '0' ? "" : "invalid ",
>+                                  dn->len);

I'm not sure this is very useful. We may want to just drop this for now.

Comment on attachment 177022 [details] [review]
Patch to glib-networking which updates gnutls implementation

needs to be updated for the API change in the other; you can still keep it in a GPtrArray internally if that's more convenient

Comment on attachment 177025 [details] [review]
Second patch to glib-networking

yup. just squash this with the previous patch though

Created attachment 177101 [details] [review]
Updated patch with dan's changes

Used G_TYPE_POINTER for property type as discussed on irc.

Created attachment 177102 [details] [review]
Updated patch for glib-networking

Squashed into a single patch.

Comment on attachment 177101 [details] [review]
Updated patch with dan's changes

good except:

>+ * Each item in the array is a #GByteArray which contains the complete

s/array/list/

>diff --git a/gio/tests/socket-client.c b/gio/tests/socket-client.c

I still think you should just pass on this part for now

Comment on attachment 177102 [details] [review]
Updated patch for glib-networking

ok with the following fixes:

>+          for (i = 0; i < gnutls->priv->accepted_cas->len; ++i)
>+            accepted_cas = g_list_prepend (accepted_cas, g_byte_array_ref (
>+                                           g_ptr_array_index (gnutls->priv->accepted_cas, i)));

Please use {}s around multi-line if/for bodies even if the multiple lines are only a single statement.

Also, g_ptr_array_index is silly. Just use "gnutls->priv->accepted_cas->pdata[i]".

>+          accepted_cas = g_list_reverse (accepted_cas);

(Does that matter? The list isn't in preference order or anything is it?)

Pushed both patches, with above changes.

Keeping the DN list in the same order it was sent doesn't really matter, but doesn't hurt either.


commit 4e33967a002fd14d7106ef2ff88122344f2e4983
Author: Stef Walter <stefw@collabora.co.uk>
Date:   Fri Dec 24 10:50:14 2010 -0600

    Change GTlsClientConnection::accepted-cas to contain DER DNs
    
    This property is now a GList of GByteArray values. Each
    GByteArray contains the raw DER DN of the certificate authority.
    This is far more useful for looking up a certificate (with the
    relevant issuer) than a string encoded DN.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=637262



commit 7e1cbb998dfd2c9dd5e1e0ad76e327ad491dc87d
Author: Stef Walter <stefw@collabora.co.uk>
Date:   Fri Dec 24 10:53:57 2010 -0600

    gnutls: Change GTlsClientConnection::accepted-cas to contain DER DNs
    
    This property is now a GList of GByteArray values. Each
    GByteArray contains the raw DER DN of the certificate authority.
    This is far more useful for looking up a certificate (with the
    relevant issuer) than a string encoded DN.
    
    Also fix memory leak in g_tls_client_connection_gnutls_retrieve_function()
    and notify when accepted-cas property has changed.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=637262

Just a drive-by comment: ;)

    This property is now a GList of GByteArray values. Each

how about using "aay" (array of array of byte) GVariant instead of this ?

(In reply to comment #14)
> Just a drive-by comment: ;)
> 
>     This property is now a GList of GByteArray values. Each
> 
> how about using "aay" (array of array of byte) GVariant instead of this ?

Yes please; actually we can't use Type: like this in documentation since the angle brackets break gtk-doc, and introspection doesn't at present handle XML-escaped attributes.  (Whether or not we should do so is a big question...)

My understanding is that GVariant is for "gvariant-based" APIs, like GSettings and GDBus, where it is useful for certain functions to be able to take arguments of arbitrary type.

Here we are not returning an object of arbitrary/unknown type, we are returning an array of bytearrays. Switching from GList to GVariant would make the function even less typesafe than it already is, while also making it less convenient to use from C. (And also un-annotatable?)

If g-i-scanner doesn't handle the currently syntax anyway, we could just change the syntax. Making it:

 * Type: GList
 * Element-Type: GByteArray

would make the property definition more parallel to the function definition anyway.

(In reply to comment #16)
> My understanding is that GVariant is for "gvariant-based" APIs, like GSettings
> and GDBus, where it is useful for certain functions to be able to take
> arguments of arbitrary type.

I'm arguing we should also use it as a general purpose GType escape hatch; why not?

> Here we are not returning an object of arbitrary/unknown type, we are returning
> an array of bytearrays. Switching from GList to GVariant would make the
> function even less typesafe than it already is,

Actually can't you annotate the type of the variant?

> while also making it less
> convenient to use from C.

Somewhat debateable =)

> (And also un-annotatable?)

You wouldn't need annotations, because the variant knows its type.

> If g-i-scanner doesn't handle the currently syntax anyway, we could just change
> the syntax. Making it:
> 
>  * Type: GList
>  * Element-Type: GByteArray
> 
> would make the property definition more parallel to the function definition
> anyway.

Could work, yes.  But honestly I'd prefer for bindings to have good GVariant capabilities (since they need them anyways) rather than trying to support weird arbitrary stuff like a GList of a GByteArray.

(In reply to comment #17)
> I'm arguing we should also use it as a general purpose GType escape hatch; why
> not?

For the same reason we wouldn't have returned a GValue containing a GValueArray of GByteArrays in older releases; because it's awkward, and you lose a degree of compile-time type safety.

> Actually can't you annotate the type of the variant?
...
> You wouldn't need annotations, because the variant knows its type.

Uh?

Anyway, AFAICT the answer to the first part is "no", because we only use GVariants in places where we don't know at annotation-writing time what the type of the argument will be. And the second part is only true for dynamic languages; static languages want as much type information at compile time as possible.

> I'd prefer for bindings to have good GVariant
> capabilities (since they need them anyways)

Sure

> rather than trying to support weird
> arbitrary stuff like a GList of a GByteArray.

What about arrays/lists of GObjects? (An earlier version of the tls code had that.)