Request for review: 4 Patches to Djabberd
Hi, I've gone and cleaned up some old patches waiting for acceptance, and I've also created some new ones. Please review and commit my patches: Issue 7732: hook phase debugging must traverse to find line number http://codereview.appspot.com/7732 Issue 2326: Memory leak in SSL connections, CTX_free needed http://codereview.appspot.com/2326 Issue 2404: [PATCH] new hook chain for unhandled stanzas http://codereview.appspot.com/2404 Issue 7915: [PATCH] send additional features from vhost in stream:features http://codereview.appspot.com/7915 thanks, Jacob
Re: method first via package B::COP
It turns out this is not a perl version issue, but rather, this problem
appears in my Djabberd development environment because a plugin makes a
callback which results in call to the fallback subroutine:
hook_chain_fast($self,
$phase,
$args || [],
$methods || {},
$fallback || sub {},
$hook_inv);
It turns out that the right way to get the line number of the callback in
this particular case is:
$cv-ROOT-first-line
instead of
$cv-ROOT-first-first-line
A proposed fix to this problem is posted at:
http://codereview.appspot.com/7732
Jacob
On Mon, Sep 22, 2008 at 5:39 AM, Brad Fitzpatrick [EMAIL PROTECTED] wrote:
Perl 5.8.8 should still be supported. This looks like somebody broke it on
accident.
Said somebody should fix. :-)
On Sat, Sep 20, 2008 at 12:54 AM, Jacob Burkhart [EMAIL PROTECTED]wrote:
Hi,
This seems to have changed as of
http://code.sixapart.com/trac/djabberd/changeset/799
With DJabberd trunk, I get
... ERROR: [Can't locate object method first via package B::COP at
lib/DJabberd/VHost.pm line 262.
after some of my plugin callbacks... and this seems to break things.
It seems package B::COP is part of the perl compiler?... is this perhaps
something that changed between perl 5.8.8 (what I'm running) and whatever
version this code was written for?
Is perl 5.8.8 still supported?
thanks,
Jacob
Re: Request for review: 4 Patches to Djabberd
Thanks, I think I like yours better. Since you catch any other possible errors as Unknown please do merge Jacob On Mon, Nov 3, 2008 at 3:13 PM, Jos I. Boumans [EMAIL PROTECTED] wrote: Hi, On Nov 3, 2008, at 8:59 PM, Jacob Burkhart wrote: I've gone and cleaned up some old patches waiting for acceptance, and I've also created some new ones. Please review and commit my patches: Issue 7732: hook phase debugging must traverse to find line number http://codereview.appspot.com/7732 Good observation; I've commited a fix for this to a branch already, but it didn't get integrated into trunk yet: http://code.sixapart.com/trac/djabberd/changeset/811 Either patch works fine from my perspective. Cheers, -- Jos Boumans Never ask a man what computer he uses. If it's a Mac, he'll tell you. If it's not, why embarrass him? - Tom Clancy CPANPLUShttp://cpanplus.sf.net
method first via package B::COP
Hi, This seems to have changed as of http://code.sixapart.com/trac/djabberd/changeset/799 With DJabberd trunk, I get ... ERROR: [Can't locate object method first via package B::COP at lib/DJabberd/VHost.pm line 262. after some of my plugin callbacks... and this seems to break things. It seems package B::COP is part of the perl compiler?... is this perhaps something that changed between perl 5.8.8 (what I'm running) and whatever version this code was written for? Is perl 5.8.8 still supported? thanks, Jacob
Re: SSL connections dropping with large messages
Yes, I am waiting on somebody to commit my patches : ) On Thu, Aug 14, 2008 at 1:50 AM, Bron Gondwana [EMAIL PROTECTED] wrote: We had an issue with SSL connections dropping with large messages, which I tracked down to being caused by the issue fixed by this patch: http://codereview.appspot.com/2341 I've applied the patch to my local copy (branch brong-dev on http://github.com/brong/brong-djabberd/ - I rebase it frequently, so don't get attached to it!) What's the status of inclusion? Jacob, are you waiting on anything? Bron. -- Bron Gondwana [EMAIL PROTECTED]
Re: Plugins to handle new types of stanzas
check out: http://codereview.appspot.com/2404 this patch introduces the hook HandleStanza which is called whenever the server receives a stanza that is not iq, presence, message or starttls. Something in the hook chain must call the 'accept' callback to signify the class to use for the stanza downbless, otherwise.. we continue on to the existing behavior which is to return a stream error. On Fri, Jun 13, 2008 at 11:04 PM, Brad Fitzpatrick [EMAIL PROTECTED] wrote: Submit a patch which add the right plugin hooks to DJabberd's core first? Then write your clean plugin? On Fri, Jun 13, 2008 at 12:41 PM, Jacob Burkhart [EMAIL PROTECTED] wrote: Hi, I am in the process of writing a plugin to support stream compression ( http://www.xmpp.org/extensions/xep-0138.html). I can't seem to find a way to setup my plugin as the handler for a new stanza type (compress) without directly editing the list of %element2class in ClientIn.pm Also, Once I receive this new stanza 'write' and 'event_read' need to be overridden on the active connection so that further communication is compressed. I can easily hack this into Connection.pm and ClientIn.pm but I want to keep the changes contained within my plugin. Any ideas? thanks, Jacob
Fwd: avoid ssl_eof error on SSL_WANT_READ and friends
-- Forwarded message -- From: [EMAIL PROTECTED] Date: Mon, Jun 23, 2008 at 2:48 PM Subject: Re: avoid ssl_eof error on SSL_WANT_READ and friends To: [EMAIL PROTECTED] Cc: [EMAIL PROTECTED] Dear bradfitz, New code review comments by Jacob have been published. Please go to http://codereview.appspot.com/2341 to read them. Message: I've uploaded a new patch Details: http://codereview.appspot.com/2341/diff/1/2 File lib/DJabberd/Stanza/StartTLS.pm (right): http://codereview.appspot.com/2341/diff/1/2#newcode74 Line 74: my $err = Net::SSLeay::get_error($ssl,-1); On 2008/06/19 20:45:39, bradfitz wrote: space after comma Done. http://codereview.appspot.com/2341/diff/1/2#newcode85 Line 85: return $err; On 2008/06/19 20:45:39, bradfitz wrote: Does this function return a boolean or an error code? The name suggests a boolean, but the return value suggests an error code. Both! It returns an error code or zero, and the return value is used as if it were a boolean in the if check in Connection to decide if we should close or not close. If we should close, we want to log the error code, so we log the value returned I've changed the function name to actual_error_on_empty_read, is that better? Open to suggestions. Issue Description: Incrementing a counter on empty SSL reads is not the correct way to determine ssl_eof (http://code.sixapart.com/trac/djabberd/changeset/758) Instead we should check the error code to see if it is one of the SSL errors for which we are supposed to retry( http://www.openssl.org/docs/ssl/SSL_get_error.html) Sincerely, Your friendly code review daemon (http://codereview.appspot.com/).
Re: DJabberd and Perl 5.10
I seem to remember some comments in the code saying something like This will break with perl 5.10... might want to search the code base for things like that. On Sat, Jun 21, 2008 at 4:29 PM, Daniel Ruoso [EMAIL PROTECTED] wrote: Hello, Today I had an unfortunate discovery. DJabberd seems to fail on Perl 5.10. At first, I couldn't really figure out what the problem is, so, before I dig into the code, did anyone solved that problem already? is there an unreleased version that works with Perl 5.10? daniel
Re: [PATCH] minor leak in StartTLS
Hi,
Please consider committing this patch to djabberd as a solution to a memory
leak we are observing in SSL connections.
thanks,
Jacob
On Thu, Mar 6, 2008 at 3:26 PM, Jacob Burkhart [EMAIL PROTECTED] wrote:
Martin,
What if we did it via Connection add_disconnect_handler? (see attached)
Instead of having Connection.pm check for a $self-{ssl} and do the
appropriate free on close, we could have StartTLS (and OldSSLClientIn) add a
callback to the close. (And so the CTX object is referenced in that
callback).
try it on for size...
Jacob
On Thu, Mar 6, 2008 at 1:18 PM, Martin Atkins [EMAIL PROTECTED]
wrote:
Jacob Burkhart wrote:
I didn't realize it was possible to have multiple VHosts. Is it
possible to have multiple certs too?
http://code.sixapart.com/trac/djabberd/browser/trunk/djabberd.conf
The example config shows the certs being configred at the top level
(outside the context of the VHost), so I assumed it was not possible to
have more than one set of certs. I'll look into it more...
Actually, I think you're right. I misread the lines of code that
reference the cert settings:
Net::SSLeay::CTX_use_certificate_file(
$ctx,
$conn-vhost-server-ssl_cert_file,
Net::SSLeay::FILETYPE_PEM
);
I didn't notice the -server in there.
However, the same thing still applies: you could potentially have
several servers running in the same process too. The stock djabberd
script doesn't do this, but djabberd can be embedded in other stuff as
well. I'm not sure what the best solution is, though. Having the SSL
context in the server object would mean that the server core would
depend on Net::SSLeay, which is a bit of a layering violation.
It seems strange to me that the cert would be server-wide, since certs
normally have the domain name in them so you'd need a different cert for
each domain and thus each vhost. Maybe I'm just misunderstanding how all
this works, though.
Would you mind cooking up a patch that just calls CTX_free at an
appropriate moment to fix the memory leak? I'll try to get hold of Brad
(who wrote this SSL stuff) and see what he thinks the SSL context object
is supposed to belong to.
ssl_ctx_r2.diff
Description: Binary data
Re: [PATCH] minor leak in StartTLS (and OldSSLClientIn)
This is a problem with OldSSLClientIn as well,here's a patch with both. thanks, Jacob On Feb 6, 2008 5:29 PM, Jacob Burkhart [EMAIL PROTECTED] wrote: There appears to me a minor memory leak in StartTLS, I observe a slow but steady climb in memory usage over the course of hundreds of client SSL connect/disconnects. Which, I no longer observe when this patch is applied. from what I can gather from: http://search.cpan.org/~sampo/Net_SSLeay.pm-1.25/SSLeay.pm The $ctx created on line 22 of: http://code.sixapart.com/trac/djabberd/browser/trunk/DJabberd/lib/DJabberd/Stanza/StartTLS.pm needs to be freed with: Net::SSLeay::CTX_free ($ctx); The other thing going on in StartTLS, is that a new $ctx is being created for every SSL connection, which is not really needed. It would be more efficient to create a single $ctx (never collect it) and reuse it. Which is another way to eliminate the need to worry about Net::SSLeay::CTX_free, and thus fix this leak. So, I submit for your consideration a patch that makes $ctx an 'our' variable, sets it when first needed, and then reuses it for every Net::SSLeay::new thanks, Jacob ssl_ctx.diff Description: Binary data
Re: [PATCH] OnPresence hook for when OnInitialPresence falls short
I no longer need this patch...I can hook into 'AlterPresenceAvailable' On Jan 30, 2008 7:31 PM, Jacob Burkhart [EMAIL PROTECTED] wrote: Hi, I submit for your review a patch to add a hook called OnPresence. My plugin needs to be notified of every single presence message (not just the one on initial connection) thanks, Jacob
Re: [PATCH] register_hook_before to complement register_hook
Hey Martin, thanks for the logconf checkin. You're right, I could just break my plugin into two parts, or perhaps even edit the hook chain directly from my plugin. I just thought the logic would be cleaner if I had this method on VHost. The plugin that needs this is actually Offline Storage. My delivery chain looks like: Local Delivery - Cluster Delivery - Offline Storage But my OnPresence chain looks like: Offline Storage - Local Delivery - Cluster Delivery This is because I'm using Djabberd as a message processing Queue. If there are offline message in the Queue for a particular JID, I'd like them all to be processed before any live messages are delivered. and now that I started to explain that... perhaps you have some ideas about another 'core' change that I've been struggling with. I'd like to modify DJabberd::Connection::ClientIn to create the field 'multi_message_delivery'. My message Q works by delivering only 1 message and then marking a connection as offline until I get another presence message from that client. Each plugin checks or sets the value of 'multi_message_delivery' based on the contents of a presence message to determine whether to act as a one message a time Queue or to deliver all messages. Can anybody imagine a way to add a field to DJabberd::Connection::ClientIn from within some sort of plugin? Jacob On Feb 3, 2008 7:18 AM, Martin Atkins [EMAIL PROTECTED] wrote: Jacob Burkhart wrote: Hi, I have a need in my plugin for an additional method on VHost.pm called register_hook_before This is because I want my offline storage plugin to be registered last in the chain for message delivery, but First in the chain for OnInitialPresence While I understand why you'd want to do this, I'm concerned that it will make the plugin mechanism more confusing for users if plugins are able to jump the queue and insert themselves at the start of the list. Right now the rule is that plugins defined earlier in the configuration file run first. This is quite easy to understand. Can you just break your plugin into two pieces and have one at the start and one at the end? What does your plugin do? Cheers, Martin
Recommended directory structure when developing Plugins and with Djabberd source
Hi, I've been working with a patched up older copy of Djabberd for a while, and now I'm working on extracting those patches into plugins and preparing some changes for submission upstream. Anyway, It looks like the version of Djabberd I'm working with has a djabberd_start.pl file that runs it, and all my plugins are in a folder called Djabberd in the same directory as djabberd_start.pl The latest copy of the source from the sixapart repository has a file called djabberd (which is basically the same as my djabberd_start.pl file) So my file system won't let me create a directory called Djabberd (for my plugins) in the same directory as the djabberd file. How do other developers have this setup, where do you put you plugins? Also, if anybody writes tests for their plugins, what directory structure do you use during development to support this? thanks, Jacob
[PATCH] --logconf option to start script
Hi, During development, I like to be able to specify a directory to my logconf file so that I don't have to store it within the directory that the rest of Djabberd sits in, and also so that I can switch between logconfs by specifying a different file (instead of having to modify a file) This patch alters the start script to accept a --logconf option, and alters Log.pm to implement set_logger. Please consider this patch for inclusion thanks, Jacob logconf.diff Description: Binary data
[PATCH] OnPresence hook for when OnInitialPresence falls short
Hi, I submit for your review a patch to add a hook called OnPresence. My plugin needs to be notified of every single presence message (not just the one on initial connection) thanks, Jacob on_presence.diff Description: Binary data
Massive leaks in StartTLS ?!?!? Net::SSLeay???
With the latest version of Djabberd from svn I'm getting a Djabberd process that grows continuously with each new connection. It appears to be related to StartTLS because I don't have this problem is I comment out my SSLCertificateFile and SSLCertificateKeyFile config directives. I must admit I know little to nothing about debugging memory leaks in perl. Any suggestions?
