Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#7432 closed patch (fixed)

BUG in ./dbus-analyze-functions.py | Dbus(libpurple-client) interface generate error (purple-client-bindings.c)

Reported by: Math2Gold Owned by: rlaager
Milestone: 2.5.3 Component: libpurple
Version: 2.5.2 Keywords: dbus
Cc: math2gold@…, ted.gould@…

Description (last modified by Math2Gold)

function purple_conversation_new always return NULL on libpurple-client.

i traced the code and checked this problem. i found this error is caused by PurpleConversationType? is converted to G_TYPE_INT on dbus client side,but it is converted to UINT32 on dbus server side.

i changed

PurpleConversation* purple_conversation_new(PurpleConversationType type, const PurpleAccount *account, const char *name) {
int RESULT_ID = 0;
dbus_g_proxy_call(purple_proxy, "PurpleConversationNew", NULL,
	G_TYPE_INT, type,  	G_TYPE_INT, GPOINTER_TO_INT(account),  	G_TYPE_STRING, name,  G_TYPE_INVALID,
	G_TYPE_INT, &RESULT_ID,  G_TYPE_INVALID);
return (PurpleConversation*) GINT_TO_POINTER(RESULT_ID);
}

to

PurpleConversation* purple_conversation_new(PurpleConversationType type, const PurpleAccount *account, const char *name) {
int RESULT_ID = 0;
dbus_g_proxy_call(purple_proxy, "PurpleConversationNew", NULL,
	G_TYPE_UINT, type,  	G_TYPE_INT, GPOINTER_TO_INT(account),  	G_TYPE_STRING, name,  G_TYPE_INVALID,
	G_TYPE_INT, &RESULT_ID,  G_TYPE_INVALID);
return (PurpleConversation*) GINT_TO_POINTER(RESULT_ID);
}

G_TYPE_INT, type changed to G_TYPE_UINT, type just.

the function works.

so i checked all PurpleConversationType? and found the same problem on client side.

as the file purple-client-bindings.c is generated by ./dbus-analyze-functions.py. it would be rewritten while i compile it again.how to recover it without change the generated file directly?


Finally , i found the bug in ./dbus-analyze-functions.py solve this problem:

Change the condition check segment on the following code

    def inputsimple(self, type, name, us):
        if us:
            self.cdecls.append("\tdbus_int32_t %s;" % name)
            self.cparams.append(("INT32", name))
            self.addintype("i", name)
        else:
            self.cdecls.append("\tdbus_uint32_t %s;" % name)
            self.cparams.append(("UINT32", name))
            self.addintype("u", name)

add not to if us:, the correct segment is if not us: (us means unsigned,so it should append UINT32 when the type is marked unsigned.)

Attachments (2)

dbus-analyze-functions.py (20.3 KB) - added by Math2Gold 11 years ago.
i recompiled pidgin,itworks.
pidgin-dbus-inputsimple-unsigned-fix.patch (1.1 KB) - added by rlaager 11 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 11 years ago by Math2Gold

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from Dbus(libpurple-client) interface generate error (purple-client-bindings.c) to BUG in ./dbus-analyze-functions.py | Dbus(libpurple-client) interface generate error (purple-client-bindings.c)
  • Type changed from defect to patch

please update it.

Changed 11 years ago by Math2Gold

i recompiled pidgin,itworks.

comment:2 Changed 11 years ago by datallah

  • Resolution fixed deleted
  • Status changed from closed to new

comment:3 Changed 11 years ago by rlaager

  • Owner set to rlaager

comment:4 Changed 11 years ago by rlaager

  • Status changed from new to pending

comment:5 follow-up: Changed 11 years ago by rlaager

I would prefer to simply re-order this code to match the rest of the file. A patch to that effect is attached, in case I'm not the one to commit this.

Thanks for your work here. Is there a real name and email address you'd like used to credit you for this change?

Changed 11 years ago by rlaager

comment:6 Changed 11 years ago by rlaager@…

  • Milestone set to 2.5.3
  • Resolution set to fixed
  • Status changed from pending to closed

(In 0d3ce6dca7222c77b50e8a23fe0ed1bae02928a8):
A patch from Math2Gold to fix the dbus-analyze-functions script to properly handle unsigned vs. signed in inputsimple. Fixes #7432

comment:7 Changed 10 years ago by darkrain42

This patch basically breaks the Dbus ABI by changing the types of a number of parameters. Ubuntu's Fast User Switching Applet trying to change the status via dbus gives me this:

method call sender=:1.5443 -> dest=:1.4769 path=/im/pidgin/purple/PurpleObject; interface=im.pidgin.purple.PurpleInterface; member=PurpleSavedstatusFindTransientByTypeAndMessage
   uint32 3
   string ""
error sender=:1.4769 -> dest=:1.5443 error_name=org.freedesktop.DBus.Error.InvalidArgs reply_serial=36
   string "Argument 0 is specified to be of type "int32", but is actually of type "uint32"
"
method call sender=:1.5443 -> dest=:1.4769 path=/im/pidgin/purple/PurpleObject; interface=im.pidgin.purple.PurpleInterface; member=PurpleSavedstatusNew
   string ""
   uint32 3
error sender=:1.4769 -> dest=:1.5443 error_name=org.freedesktop.DBus.Error.InvalidArgs reply_serial=37
   string "Argument 1 is specified to be of type "int32", but is actually of type "uint32"
"
method call sender=:1.5443 -> dest=:1.4769 path=/im/pidgin/purple/PurpleObject; interface=im.pidgin.purple.PurpleInterface; member=PurpleSavedstatusGetCurrent
method return sender=:1.4769 -> dest=:1.5443 reply_serial=38
   int32 4374
method call sender=:1.5443 -> dest=:1.4769 path=/im/pidgin/purple/PurpleObject; interface=im.pidgin.purple.PurpleInterface; member=PurpleSavedstatusGetType
   int32 4374
method return sender=:1.4769 -> dest=:1.5443 reply_serial=39
   int32 5

Disapproving the patch and recompiling returns it to functioning.

comment:8 Changed 10 years ago by deryni

  • Resolution fixed deleted
  • Status changed from closed to new

Re-opening this so we can decide if we need to revert this or not.

comment:9 Changed 10 years ago by rlaager

I'm not sure how I feel about this. The types were 100% wrong before and this change fixes them. On the other hand, it does break compatibility. I'm going to ask Ted Gould, who wrote that code in the FUSA applet to see if he has any input.

comment:10 Changed 10 years ago by deryni

I'm ok deciding that we want to keep the patched version, we just need to make sure we sort it out with the FUSA people (which shouldn't be too hard), we may even want to have a blog post about it. If we decide we want to revert we may want to consider doing so 'soon' since we don't want to break anything that decides to update to match or gets written from scratch now.

comment:11 Changed 10 years ago by rlaager

I'm of the opinion that we should fix this bug (i.e. keep the patch), regressions be damned. Basically, either way, some caller isn't going to work. I don't think the FUSA changes have made it upstream, so it seems like now is the best time to deal with this.

comment:12 Changed 10 years ago by rlaager

  • Cc ted.gould@… added

Ted Gould wrote:

Richard Laager wrote:

Right now, are the status changes in FUSA upstream, or just in Ubuntu? If they're just in Ubuntu, coordinating the patch to Pidgin and FUSA shouldn't be hard.

If the changes are upstream, have they released yet? If so, then I'd say we should just coordinate the changes in Pidgin to match a FUSA release. If not, let's just get it changed now.

They aren't currently, but I've talked to upstream and we're going to merge them in this cycle. That would put the release as the next GNOME release which is ~Feb. But, they would generally expect the devel builds to build against stable dependencies right now... perhaps we'll just have to handle the two cases of Pidgin versions.

Ted, we're looking at cutting a release on Thursday. This would be the first version to use the corrected integer types. Would that satisfy the GNOME folks?

comment:13 Changed 10 years ago by deryni

2.5.3 has been out since Dec. 20th and had this patch applied. The discussion here was about whether to revert the patch since people may have come to depend on the broken types. Unless I've missed something along the way.

comment:14 Changed 10 years ago by rlaager

  • Resolution set to fixed
  • Status changed from new to closed

I didn't realize we had already released the change. Anyway, Ted wrote back to me, "Yeah, I think apply the patch. We'll make it work out." So I'm going to consider this issue closed, unless we should add a bit to the ChangeLog.API file?

comment:15 in reply to: ↑ 5 Changed 10 years ago by Math2Gold

Replying to rlaager:

I would prefer to simply re-order this code to match the rest of the file. A patch to that effect is attached, in case I'm not the one to commit this.

Thanks for your work here. Is there a real name and email address you'd like used to credit you for this change?

Sorry for i'm late to reply.Glad to know that this bug fixed.For some reasons,i have no time to check mail one by one in past several months.The email is real,of couse.i used it to reset my password.But math2gold is not real name. The real name is F.W. Kong .i'm glad to if is still available to credit me.thanks:)

BTW:i'd like to make some comments on the document for make it easier to read.what or which way is better?sorry for my stupid question.Should this discussion move to any right place?

comment:16 Changed 10 years ago by deryni

If you want to make further changes to the script to make it easier for people to follow feel free to do so and file new patch tickets with them. If you want to discuss any such changes feel free to send mail to the devel mailing list.

comment:17 Changed 10 years ago by rlaager

I credited you in the COPYRIGHT file. (For the record, since I forgot the "Refs #7432", it was aa0c4c79c82ee967ae25bd40361b53dd60a84e2b.)

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!