Summary

start of the new xmpp's connection

Review Request #2866 — Created Dec. 4, 2023 and submitted Dec. 12, 2023, 7:23 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

Depends On

Reviewers

Groups

pidgin

People

Description

Right now this just establishes a TCP connection via SRV record and nothing
more.

Testing Done

Connected to pidginchat.com successfully and verified the traffic via charles proxy.

Commits

Summary	ID
start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	4e897d4341e0f24255330426bc164101842eeeef

Issues

Description	From	Last Updated
After a bunch of thought, it seems like moving the actual connection stuff into Xeme is the ideal way to …	grim	Dec. 8, 2023, 6:20 a.m.
Did you mean to do *error == NULL?	ivanhoe	Dec. 12, 2023, 7:06 a.m.
local_error must be non-NULL, which is not guaranteed by the condition above.	ivanhoe	Dec. 12, 2023, 7:06 a.m.
leak	ivanhoe	Dec. 12, 2023, 6:32 a.m.
This appears to need clearing in dispose or finalize?	QuLogic	Dec. 12, 2023, 5:31 a.m.
Need to clear resolver in here, as this condition could still be true with a resolver, but local_error != NULL.	QuLogic	Dec. 12, 2023, 5:31 a.m.
Space around ** in wrong spot.	QuLogic	Dec. 12, 2023, 5:31 a.m.

Change Summary:

Make tls a list and use constants for all of the options

Commits:

	Summary	ID
	start of the new xmpp's connection Right now this just establishes a TCP connection via SRV record and nothing more.	2389ff297dc65b7e9b17922db6de4c3bb5dd1701
	start of the new xmpp's connection Right now this just establishes a TCP connection via SRV record and nothing more.	9dcad243290133606c9210aff548627b5d363199

Diff:

Revision 2 (+852)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	protocols/xmpp/purplexmppxeps.h

Change Summary:

remove the default resource stuff as we don't need it yet.

Commits:

	Summary	ID
	start of the new xmpp's connection Right now this just establishes a TCP connection via SRV record and nothing more.	9dcad243290133606c9210aff548627b5d363199
	start of the new xmpp's connection Right now this just establishes a TCP connection via SRV record and nothing more.	052c9a314399c924436d338746528982fa72710e

Diff:

Revision 3 (+832)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	protocols/xmpp/purplexmppxeps.h

Change Summary:

Added fallbacks for tls srv and clear srv and wired up xeme

Testing Done:

~		Connected to pidginchat.com successfully.
	~	Connected to pidginchat.com successfully and verified the traffic via charles proxy.

Commits:

	Summary	ID
	start of the new xmpp's connection Right now this just establishes a TCP connection via SRV record and nothing more.	052c9a314399c924436d338746528982fa72710e
	start of the new xmpp's connection This wires up the Xeme input and output streams but a lot more work needs to be done on the xeme side to proceed.	39ef1121fef811b53ec50d6b2caeb0d890132e39

Diff:

Revision 4 (+1410)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	protocols/xmpp/purplexmppxeps.h

Show all issues

After a bunch of thought, it seems like moving the actual connection stuff into Xeme is the ideal way to go because making everyone implement this is insane.

I'll leave this open for now as reference, but by adding a XemeConnection object we can set properties and stuff on it from the account options and then call XemeConnection.connect_async to start the actual process and then obvious XemeConnection.connect_finish to get the results.

Change Summary:

migrate to the new XemeConnection object

Commits:

	Summary	ID
	start of the new xmpp's connection This wires up the Xeme input and output streams but a lot more work needs to be done on the xeme side to proceed.	39ef1121fef811b53ec50d6b2caeb0d890132e39
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	9b405ce7ec456d8e30be6cb596721099da58b12a

Diff:

Revision 5 (+746 -2)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	subprojects/xeme.wrap

protocols/xmpp/purplexmppconnection.c (Diff revision 5)
Show all issues
```
Did you mean to do *error == NULL?
```

protocols/xmpp/purplexmppconnection.c (Diff revision 5)

Show all issues

local_error must be non-NULL, which is not guaranteed by the condition above.

protocols/xmpp/purplexmppconnection.c (Diff revision 5)

Show all issues

leak

grim Dec. 12, 2023, 2:28 a.m.

This is cleared on line 150 right below. If you saw this leaking in valgrind it has to be in xeme and thus not related to this review request.

Change Summary:

rebased and addressed error handling stuff

Commits:

	Summary	ID
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	9b405ce7ec456d8e30be6cb596721099da58b12a
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	4962121a548bb6a2912ec4d6fd47f115ac6215a4

Diff:

Revision 6 (+746 -2)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	subprojects/xeme.wrap

protocols/xmpp/purplexmppconnection.c (Diff revision 6)
Show all issues
```
This appears to need clearing in dispose or finalize?
```

protocols/xmpp/purplexmppconnection.c (Diff revision 6)

Show all issues

Need to clear resolver in here, as this condition could still be true with a resolver, but local_error != NULL.

protocols/xmpp/purplexmppprotocol.c (Diff revision 6)
Show all issues
```
Space around ** in wrong spot.
```

Change Summary:

address more issues

Commits:

	Summary	ID
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	4962121a548bb6a2912ec4d6fd47f115ac6215a4
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	71be100ade7883954e32c9969159374151a7d0e9

Diff:

Revision 7 (+752 -2)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	subprojects/xeme.wrap

Ship it!

```
Ship It!
```

Change Summary:

depend on the latest version of xeme that fixes a memory leak.

Commits:

	Summary	ID
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	71be100ade7883954e32c9969159374151a7d0e9
	start of the new xmpp's connection This uses XemeConnection to do all of the heavy lifting.	4e897d4341e0f24255330426bc164101842eeeef

Diff:

Revision 8 (+752 -2)

Show changes

	protocols/xmpp/meson.build
	protocols/xmpp/purplexmppconnection.h
	protocols/xmpp/purplexmppconnection.c
	protocols/xmpp/purplexmppconstants.h
	protocols/xmpp/purplexmppcore.c
	protocols/xmpp/purplexmppprotocol.c
	subprojects/xeme.wrap

Ship it!

```
Ship It!
```

Review Board 6.0.2

start of the new xmpp's connection

Information

Reviewers

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Testing Done:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Change Summary:

Commits:

Diff:

Status: Closed (submitted)