Created attachment 114024 [details] [review]
gmime24.patch

Hi,

Thank you for this patch.
I went ahead to update to gmime-2.4.3 here (MacOSX Leopard).
I have now indeed hit the problem with g_mime_message_get_body.  I wonder if you could give us a clue how to copy it over from gmime-2.2.x and let pan2 continue, please?  (C is ok, c++ ain't, with me ;) ).
[…]
llvm-g++-4.2 -DHAVE_CONFIG_H -I. -I../..  -I../.. -I/usr/local/include/gmime-2.4 -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include   -D_REENTRANT -I/usr/local/include/glib-2.0 -I/usr/local/lib/glib-2.0/include   -I/usr/local/include -I/usr/X11R6/include -I/Developer/Headers/FlatCarbon -I/usr/include   -Os  -g  -mtune=core2 -march=core2 -force_cpusubtype_ALL  -arch i386  -arch x86_64  -isysroot /Developer/SDKs/MacOSX10.5.sdk  -c -o message-check.o message-check.cc
message-check.cc: In static member function ‘static void pan::MessageCheck::message_check(const GMimeMessage*, const pan::StringView&, const pan::quarks_t&, std::set<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&, pan::MessageCheck::Goodness&)’:
message-check.cc:443: error: ‘g_mime_message_get_body’ was not declared in this scope
message-check.cc:473: error: invalid conversion from ‘const InternetAddressList*’ to ‘InternetAddressList*’
message-check.cc:473: error:   initializing argument 1 of ‘int internet_address_list_length(InternetAddressList*)’
message-check.cc: In static member function ‘static void pan::MessageCheck::message_check(const GMimeMessage*, const pan::StringView&, const pan::quarks_t&, std::set<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::less<std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > >&, pan::MessageCheck::Goodness&)’:
message-check.cc:443: error: ‘g_mime_message_get_body’ was not declared in this scope
message-check.cc:473: error: invalid conversion from ‘const InternetAddressList*’ to ‘InternetAddressList*’
message-check.cc:473: error:   initializing argument 1 of ‘int internet_address_list_length(InternetAddressList*)’
lipo: can't figure out the architecture type of: /Volumes/RamDisk/tmp/ccviTIzA.out
make[3]: *** [message-check.o] Error 1
make[3]: Leaving directory `/Volumes/ProjectsTodoStuff/Projects/gnome_cvs/pan2/pan/usenet-utils'
[…]

btw uh yeah I'm on the bleeding edge with Apple's llvm based on gcc-4.2.  ;)  I also went up with glib-2.19.0.  I did invent a patch to allow it to compile pan/general/worker-pool.cc, before I forced pan2 to use your patch here (was using gmime-2.2.17 before).  Once we jump over this g_mime_message_get_body hurdle, I'll be able to test then submit a bugreport for worker-pool.

Thank you for any help.

pretty busy at the moment, I'll try to make time for it in a week or so

Created attachment 126025 [details] [review]
speculative patch

Here's an updated patch with some extra copy and pasting of gmime-2.2 bits and some other guess work. It gets pan to build at least, though I haven't actually run it or tested it to see if it makes any real sense. But it might be useful even if unsuccessful

doh, forgot all about this. (unfortunately still bogged down at work)

Lemme give a few suggestions, tho, while I drink my morning coffee:

1. would be best to prefix the ported gmime functions with pan_ - so for example, pan_g_mime_message_get_body() or some such. That's the way charles used to do it, altho he may have a newer convention now that pan is c++? This is to safeguard pan in case some future version of GMime gets new functions with identical symbol names (I doubt it, but in the off chance...)

2. pan's g_mime_part_get_content() should be fixed to always return an allocated buffer. Don't return a const pointer sometimes and an allocated buffer other times. Basically just keep the logic that writes the content to a memstream.

It may even be a good idea to just return a GByteArray instead of a char*, depending on how the code that calls g_mime_message_get_body() uses it (I suspect it'll clean up the code to just return a GByteArray).

I just tried the patch from comment #4 as Pan is broken on Fedora Rawhide (due to gmime 2.4). The program starts fine, but as soon as I try to read a message from a newsgroup, the program spits out critical warnings and eventually segfaults:

(pan:7253): gmime-CRITICAL **: g_mime_stream_eos: assertion `GMIME_IS_STREAM (stream)' failed

(pan:7253): gmime-CRITICAL **: g_mime_stream_reset: assertion `GMIME_IS_STREAM (stream)' failed

(pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

(pan:7253): gmime-CRITICAL **: g_mime_data_wrapper_write_to_stream: assertion `GMIME_IS_DATA_WRAPPER (wrapper)' failed

(pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

(pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

(pan:7253): gmime-CRITICAL **: g_mime_stream_eos: assertion `GMIME_IS_STREAM (stream)' failed

(pan:7253): gmime-CRITICAL **: g_mime_stream_reset: assertion `GMIME_IS_STREAM (stream)' failed

(pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

(pan:7253): gmime-CRITICAL **: g_mime_data_wrapper_write_to_stream: assertion `GMIME_IS_DATA_WRAPPER (wrapper)' failed

(pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed

Unfortunately I wasn't able to get a stacktrace of it..probably because of a kinda broken kernel in Fedora Rawhide

Confirming bug.  Lack of a pan patch means that the rawhide package is causing broken deps in Fedora:

Broken deps for i386
----------------------------------------------------------
	1:pan-0.133-1.fc10.i386 requires libgmime-2.0.so.2


(from http://koji.fedoraproject.org/mash/rawhide-20090314/logs/depcheck )

We may have to block the package from the f11-beta, so it would be good if there was a working patch we could apply.

Confirming bug. macports updated gmime recently -> pan broke
http://trac.macports.org/ticket/20741

13 month and nothing done. And Charles Kerr has not said a word yet. I guess thinks look bleak for this project.

If you look back in time, Charles' commit pattern has always been fits and starts.  He'll go great guns for awhile (he was doing near-weekly betas for over a year the last time), then leave it untouched for a year, two years, perhaps three...  Last time he went for a long time, before that year plus of "great guns", he came back with a surprise, a full pan rewrite, from C into C++.  The new version was much more scalable and had a number of very good new features, tho it still lacks one or two of the older features.

So this is nothing unusual for him.  Whether he'll come back to it or not I don't know, but this isn't unusual.  It'd be nice if pan had a team of developers that could care for it, such that Charles could come in and go great guns when he wants to, but development wouldn't seem to stop for ages in between.

Meanwhile, you're welcome to come join the user list.  It's listed on pan's home page (pan.rebelbase.com), and available thru gmane, if you wish to participate in it as a newsgroup (as I do).  One of the regulars there, K. Haley, has done a git-clone of the Gnome repository, and is making regular commits, mostly based on the patches filed here, so far.  I don't know where it'll lead, but currently, grabbing a clone of his git repository and doing regular git-pulls, then building from source, is the way to keep current.  But that's not the sort of thing you want to do without a support and discussion forum, so you do really want to join the user list if you're going to go that route.

Duncan

(In reply to comment #6)
> I just tried the patch from comment #4 as Pan is broken on Fedora Rawhide (due
> to gmime 2.4). The program starts fine, but as soon as I try to read a message
> from a newsgroup, the program spits out critical warnings and eventually
> segfaults:
> 
> (pan:7253): gmime-CRITICAL **: g_mime_stream_eos: assertion `GMIME_IS_STREAM
> (stream)' failed
> 
> (pan:7253): gmime-CRITICAL **: g_mime_stream_reset: assertion `GMIME_IS_STREAM
> (stream)' failed
> 
> (pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT
> (object)' failed
> 

The errors above seem to be because of a relationship between two different objects.  This is the following code in mime-utils.cc handle_uu_and_yenc_in_text_plain.

    // we assume that inlined yenc and uu are only in text/plain blocks
    GMimeContentType * content_type = g_mime_object_get_content_type (*part);
    if (!g_mime_content_type_is_type (content_type, "text", "plain"))
      return;

    // get this part's content
    GMimeDataWrapper * content = g_mime_part_get_content_object (GMIME_PART (*part));
    if (!content || !GMIME_IS_DATA_WRAPPER(content))
      return;

    // wrap a buffer stream around it for faster reading -- it could be a file stream
    GMimeStream * stream = g_mime_data_wrapper_get_stream (content);
    g_mime_stream_reset (stream);
    GMimeStream * istream = g_mime_stream_buffer_new (stream, GMIME_STREAM_BUFFER_BLOCK_READ);
    g_object_unref (stream);
    g_object_unref (content);

    // break it into separate parts for text, uu, and yenc pieces.
    temp_parts_t parts;
    separate_encoded_parts (istream, parts);
    g_mime_stream_reset (istream);

There are also cases where g_mime_part_get_content_object doesn't return null but it also doesn't return a GMimeDataWrapper either.  For example the following results in an assertion g_mime_data_wrapper_write_to_stream GMIME_IS_DATA_WRAPPER fails for wrapper.  From body-pane.cc get_pixbuf_from_gmime_part.

    GMimeDataWrapper * wrapper = g_mime_part_get_content_object (part);
    if (wrapper)
    {
      GMimeStream * mem_stream (g_mime_stream_mem_new ());
      g_mime_data_wrapper_write_to_stream (wrapper, mem_stream);

Change the if statement to "if (wrapper && GMIME_IS_DATA_WRAPPER(wrapper))" and assertions go away.

(In reply to comment #11)
Somehow lost the comment that if you comment out "g_object_unref(stream), istream passed to stream_readln eventually, no longer fails the GMIME_IS_STREAM test for g_mime_stream_eos.

> (In reply to comment #6)
> > I just tried the patch from comment #4 as Pan is broken on Fedora Rawhide (due
> > to gmime 2.4). The program starts fine, but as soon as I try to read a message
> > from a newsgroup, the program spits out critical warnings and eventually
> > segfaults:
> > 
> > (pan:7253): gmime-CRITICAL **: g_mime_stream_eos: assertion `GMIME_IS_STREAM
> > (stream)' failed
> > 
> > (pan:7253): gmime-CRITICAL **: g_mime_stream_reset: assertion `GMIME_IS_STREAM
> > (stream)' failed
> > 
> > (pan:7253): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT
> > (object)' failed
> > 
> 
> The errors above seem to be because of a relationship between two different
> objects.  This is the following code in mime-utils.cc
> handle_uu_and_yenc_in_text_plain.
> 
>     // we assume that inlined yenc and uu are only in text/plain blocks
>     GMimeContentType * content_type = g_mime_object_get_content_type (*part);
>     if (!g_mime_content_type_is_type (content_type, "text", "plain"))
>       return;
> 
>     // get this part's content
>     GMimeDataWrapper * content = g_mime_part_get_content_object (GMIME_PART
> (*part));
>     if (!content || !GMIME_IS_DATA_WRAPPER(content))
>       return;
> 
>     // wrap a buffer stream around it for faster reading -- it could be a file
> stream
>     GMimeStream * stream = g_mime_data_wrapper_get_stream (content);
>     g_mime_stream_reset (stream);
>     GMimeStream * istream = g_mime_stream_buffer_new (stream,
> GMIME_STREAM_BUFFER_BLOCK_READ);
>     g_object_unref (stream);
>     g_object_unref (content);
> 
>     // break it into separate parts for text, uu, and yenc pieces.
>     temp_parts_t parts;
>     separate_encoded_parts (istream, parts);
>     g_mime_stream_reset (istream);
> 
> There are also cases where g_mime_part_get_content_object doesn't return null
> but it also doesn't return a GMimeDataWrapper either.  For example the
> following results in an assertion g_mime_data_wrapper_write_to_stream
> GMIME_IS_DATA_WRAPPER fails for wrapper.  From body-pane.cc
> get_pixbuf_from_gmime_part.
> 
>     GMimeDataWrapper * wrapper = g_mime_part_get_content_object (part);
>     if (wrapper)
>     {
>       GMimeStream * mem_stream (g_mime_stream_mem_new ());
>       g_mime_data_wrapper_write_to_stream (wrapper, mem_stream);
> 
> Change the if statement to "if (wrapper && GMIME_IS_DATA_WRAPPER(wrapper))" and
> assertions go away.

> There are also cases where g_mime_part_get_content_object doesn't return null
> but it also doesn't return a GMimeDataWrapper either.  For example the
> following results in an assertion g_mime_data_wrapper_write_to_stream
> GMIME_IS_DATA_WRAPPER fails for wrapper.  From body-pane.cc
> get_pixbuf_from_gmime_part.
> 
>     GMimeDataWrapper * wrapper = g_mime_part_get_content_object (part);
>     if (wrapper)
>     {
>       GMimeStream * mem_stream (g_mime_stream_mem_new ());
>       g_mime_data_wrapper_write_to_stream (wrapper, mem_stream);
> 
> Change the if statement to "if (wrapper && GMIME_IS_DATA_WRAPPER(wrapper))" and
> assertions go away.

g_mime_part_get_content_object() can only ever return a GMimeDataWrapper or null. However, if the message was parsed by the parser, then null should never be returned. The only way it can return null is if the GMimePart was constructed outside of the parser and no content was set on the part.

What is likely happening is some unbalanced ref/unref operations.

I should note that one of the changes in GMime 2.4 is that accessor methods on objects no longer return ref'd objects.

E.g. you should not unref the return value of g_mime_part_get_content_object().

My initial patch porting Pan to 2.4 may have missed some places.

Created attachment 143086 [details] [review]
Another attempt mostly commenting out unrefs

Search for SKG in the diffs.  There are 11 changes.  Mostly commenting out unref calls to data wrappers which when gotten via g_mime_part_get_content_object shouldn't be unrefed.  Commented out one returned const char * which shouldn't be freed according to gmime 2.4 FAQ.

Commented out two GMimeStream unrefs because the stream was used to create another stream.  Unrefing both makes the stream in the data wrapper the first was retrieved from invalid.

OOPS.  Commenting out of the g_free for pch in body-pane.cc line 955 was a mistake.

FYI, I'm planning on doing a maintenance release of Pan sometime in the next few months to remove the deprecated glib/gtk calls, start using GRegex instead of pcre, fix a few gcc build errors, etc.

To be honest I'm not sure how much time I have to work on this GMime patch, but if the two of you get it hammered out to where you're both happy with it, I'll use it... this is right in line with the other changes I'm working on to remove crufty dependencies.

An updated version of this patch is in my repo it was based on the speculative patch.  I've already pulled out the unrefs as well as making several other changes.  Unfortunately I'm just now remembering to update bugzilla.

git://github.com/lostcoder/pan2.git

K & Scott: is the current patch ready, or is it still a work in progress?

Well, the last bug I fixed was to re-write multipart handling for display.  That was over a month ago.  No one has mentioned any problems since then.

Created attachment 144228 [details] [review]
new patch

Here is the consolidated patch from my tree.  It's based on commit 647b8c2 from your tree.

(In reply to comment #19)
> K & Scott: is the current patch ready, or is it still a work in progress?

There is one thing I haven't figured out yet having to do with yenc or uuencoded messages.  In BodyPane :: foreach_part_cb (GMimeObject* /*parent*/, GMimeObject* o, gpointer self), we first come through and test to see if the object is multipart with "if (GMIME_IS_MULTIPART (o))" and it is.  Then "g_mime_multipart_foreach (GMIME_MULTIPART (o), foreach_part_cb, self)" is called.  For some reason I haven't figured out, this causes BodyPane::foreach_part_cb to run twice with objects that are not multipart.  The result is that the decoded data is displayed twice in view.  Two duplicate pictures or whatever was encoded.  I added a log message to dump the number of parts if it is multipart and it claims there is only one part but the callback is run twice.

printf("%s, %d parts %d\n", __FILE__, __LINE__, g_mime_multipart_get_count((GMimeMultipart *) o));

This may have something to do with the way "void handle_uu_and_yenc_in_text_plain (GMimeObject **part)" replaces the encoded part with the decoded part.  I only see this in encoded messages.  A text post is only displayed once.

Hey Scott...

GMime 2.4's message/multipart foreach functions changed semantics a bit. They are now recursive (whereas with 2.2.x they were not).

This might be what you are experiencing...

Scott,

Unless I missed an unref somewhere and no one has hit that code, the patch I posted last night fixes all of that.  It also includes my re-write of the multipart handling.

Comment on attachment 144228 [details] [review]
new patch

The current version of this patch can be found in my repo in the gmime branch.  It has been updated to work with the current code from gnome.

There is also an integration branch that has all of my patches merged in.

git://github.com/lostcoder/pan2.git

Any news on this? This is more urgent now since gmime-2.2 is no longer supported:
https://bugzilla.gnome.org/show_bug.cgi?id=614025#c2

Thanks

I have no idea if / when any of the patches will make it into the main pan repo.  Until then you will need to use the version from my repo.  Both the master and testing branches support gmime-2.4 & gmime-2.5.  Also iirc the version in the main repo has a serious bug in its newsrc writing that affects its ability to track read articles.

What I think should happen, is that haleykd should get write access to GNOME's repo and take over maintenance of Pan.  It's unlikely that I'm going to be working more on Pan anytime soon, as I rarely use Usenet anymore.

(In reply to comment #28)
> What I think should happen, is that haleykd should get write access to GNOME's
> repo and take over maintenance of Pan.

As Eric S Raymond points out in "The Cathedral and the Bazaar" (which I read in the collected essays book form shortly after switching to Linux and which had quite a dramatic effect on my understanding of the entire FLOSS movement), in the FLOSS world:

"5. When you lose interest in a program, your last duty to it is to hand it off to a competent successor."

While I'm not sure how interested he is in actually taking that responsibility, haleykd has certainly demonstrated the competence and as importantly, taken the initiative.  If he's willing, I'm for it, and I don't expect there'll be much resistance on the user list.  (Heh, resistance?  A lot of the list is already compiling from his git repo.)

Duncan: well, I thought I made it pretty clear 9 months ago that I didn't have anything cooking in secret during this hiatus, and that there needed to be more people involved in the project so that it could survive me going idle.

However I didn't officially hand the baton over, which is the logical next step.  And doing so would probably be a lot healthier for Pan than the way things are now.

K Haley, you're the logical choice -- you're already the de facto primary developer, and more importantly, your patches have been good.  If you want to make it official and be the primary developer for Pan, you've got my blessing.

I've been trying to contact Matt about the website for a couple of weeks now, but haven't heard back from him.  So I don't know if you would want to continue hosting at pan.rebelbase.com, or to move it to some other site.  Either way would be fine with me.

Best of luck.  I put a lot of work into Pan and I'm glad that it's survived without me.

Charles

A side thought: it's possible that Chris still has all the passwords stored somewhere for the ML admin and the website.  I don't have them anymore, and Matt appears to be AWOL, so contacting Chris would be a good First Step in deciding whether or not to rehost everything.

I would prefer to have someone else act a final gateway for the patches.  Especially since I only have two or three more cooking.  Everything else is ,I think, done.

As for the website, someone else should definitely handle that.  Maybe Duncan since he's been far more active in the community I have.

That would be fine too.

In the time since my last comment I've gotten a partial response from Matt, so I should soon have the info I need to dust off my old rebelbase account.  It would probably be easiest to keep that site, since its URL has been in circulation for over a decade.(!)  /If/ that works out, then I'd be happy enough to upload the updates, such as new tarballs.  Actually even better would be to get accounts other volunteers too.

Perhaps a 0.134 should be rolled soon, to show the world signs of life?  Unfortunately I've let the traditional (though entirely coincidental) August 1st release date slip past, but that's no reason to not get all the recent improvements into an official tarball.

IMO K Haley's github repo is the official one now, rather than the de facto unofficial one...

I think that it's important now to not let the official project die out, and to resume Pan development within the official repository & infrastructure. For me as a Pan user, it's great to hear from Charles and about his willingness to sponsor K Haley's request for a GNOME commit account, and hand over the Pan maintainership.

K Haley, in order to get your account created, could you please follow the instructions on the following page:

http://live.gnome.org/NewAccounts

When sending your request, it'd probably be a good idea to refer to this bug report with Charles stating his approval of you as a new official Pan developer/maintainer. Note that the application usually takes some time to be fully processed by GNOME sysadmins.

As for me, I'm definitely willing to help you guys out, but my skills lies in documentation writing & localization, so unfortunately I can't do the much needed development work. For what it's worth, I have had my own GNOME commit account for several years now, so I can assist in comitting some simple patches etc. to the Pan repository.

If nobody else steps in, I should be able to help with the website stuff, or to manage migration to e.g. projects.gnome.org, as with other projects hosted on GNOME infrastructure, if needed.

Let's get the project some new momentum!

Merged into GNOME master. Thank you guys for all your work!