Bug 30302

> @@ -401,14 +283,13 @@ get_properties_reply_cb (GObject *source,
> 
>    if (!check_query_reply_msg (reply_msg, NULL))
>      {
> -      const gchar *buddy;
> +      GError err = { TP_ERRORS, TP_ERROR_NETWORK_ERROR,
> +        "Server returned an error" };
>  
> -      buddy = lm_message_node_get_attribute (
> -          wocky_stanza_get_top_node (reply_msg), "from");
> -      g_assert (buddy != NULL);
> +      DEBUG ("Query failed: %s", error->message);
>  
> -      DEBUG ("PEP query failed. Let's try to search this buddy.");
> -      get_buddy_properties_from_search (ctx->conn, buddy, ctx->context);
> +      dbus_g_method_return_error (ctx->context, &err);
> +      g_error_free (error);
>        goto out;

Fairly serious copypasta error: @error isn't set, so the DEBUG() is a null pointer dereference!

The second argument of check_query_reply_msg is in fact a DBusGMethodInvocation, so I think you can streamline this to:

    if (!check_query_reply_msg (reply_msg, ctx->context))
      {
        DEBUG ("Query failed");
        goto out;
      }

> @@ -446,18 +326,6 @@ olpc_buddy_info_get_properties (GabbleSvcOLPCBuddyInfo *iface,
...
> -  /* First check if we can find properties in a buddy view */
...
>    /* Then try to query the PEP node */

The second comment doesn't make sense without the first; delete it.

(In reply to comment #1)
> > @@ -401,14 +283,13 @@ get_properties_reply_cb (GObject *source,
> > 
> >    if (!check_query_reply_msg (reply_msg, NULL))
> >      {
> > -      const gchar *buddy;
> > +      GError err = { TP_ERRORS, TP_ERROR_NETWORK_ERROR,
> > +        "Server returned an error" };
> >  
> > -      buddy = lm_message_node_get_attribute (
> > -          wocky_stanza_get_top_node (reply_msg), "from");
> > -      g_assert (buddy != NULL);
> > +      DEBUG ("Query failed: %s", error->message);
> >  
> > -      DEBUG ("PEP query failed. Let's try to search this buddy.");
> > -      get_buddy_properties_from_search (ctx->conn, buddy, ctx->context);
> > +      dbus_g_method_return_error (ctx->context, &err);
> > +      g_error_free (error);
> >        goto out;
> 
> Fairly serious copypasta error: @error isn't set, so the DEBUG() is a null
> pointer dereference!
> 
> The second argument of check_query_reply_msg is in fact a
> DBusGMethodInvocation, so I think you can streamline this to:
> 
>     if (!check_query_reply_msg (reply_msg, ctx->context))
>       {
>         DEBUG ("Query failed");
>         goto out;
>       }

True, check_query_reply_msg already calls dbus_g_method_return_error and as it also logs the error I'm omitting it from the caller.

> > @@ -446,18 +326,6 @@ olpc_buddy_info_get_properties (GabbleSvcOLPCBuddyInfo *iface,
> ...
> > -  /* First check if we can find properties in a buddy view */
> ...
> >    /* Then try to query the PEP node */
> 
> The second comment doesn't make sense without the first; delete it.

Done.

review+, although it would have been easier to review if you hadn't squashed the fixes into the initial patch (I assume you didn't make any other changes than the ones I suggested).