[PATCH] Setup the GMimeStream only when needed

[PATCH] Setup the GMimeStream only when needed

[nstraz]

I ran into this while looking at the vim plugin.  Vim's system() call
redirects output to a file and it was missing many of the part{ lines.

If stream_stdout is setup too early, it will overwrite the part start
when notmuch is redirected to a file.
---
 notmuch-show.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/notmuch-show.c b/notmuch-show.c
index ff1fecb..96647c1 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -236,9 +236,6 @@ format_part_text (GMimeObject *part, int *part_count)
 {
     GMimeContentDisposition *disposition;
     GMimeContentType *content_type;
-    GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);
-
-    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
 
     disposition = g_mime_object_get_content_disposition (part);
     if (disposition &&
@@ -256,14 +253,14 @@ format_part_text (GMimeObject *part, int *part_count)
 	if (g_mime_content_type_is_type (content_type, "text", "*") &&
 	    !g_mime_content_type_is_type (content_type, "text", "html"))
 	{
+	    GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);
+	    g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
 	    show_part_content (part, stream_stdout);
+	    g_object_unref(stream_stdout);
 	}
 
 	printf ("\fattachment}\n");
 
-	if (stream_stdout)
-	    g_object_unref(stream_stdout);
-
 	return;
     }
 
@@ -276,7 +273,10 @@ format_part_text (GMimeObject *part, int *part_count)
     if (g_mime_content_type_is_type (content_type, "text", "*") &&
 	!g_mime_content_type_is_type (content_type, "text", "html"))
     {
+	GMimeStream *stream_stdout = g_mime_stream_file_new (stdout);
+	g_mime_stream_file_set_owner (GMIME_STREAM_FILE (stream_stdout), FALSE);
 	show_part_content (part, stream_stdout);
+	g_object_unref(stream_stdout);
     }
     else
     {
@@ -285,9 +285,6 @@ format_part_text (GMimeObject *part, int *part_count)
     }
 
     printf ("\fpart}\n");
-
-    if (stream_stdout)
-	g_object_unref(stream_stdout);
 }
 
 static void
-- 
1.6.6.1

Re: [PATCH] Setup the GMimeStream only when needed

[Michal Sojka]

On Thu, 01 Apr 2010, nstraz@redhat.com wrote:
> I ran into this while looking at the vim plugin.  Vim's system() call
> redirects output to a file and it was missing many of the part{ lines.
> 
> If stream_stdout is setup too early, it will overwrite the part start
> when notmuch is redirected to a file.

Hi,

thanks for the patch. After some investigation, it seems that you are
right. What I missed in your commit message is the reason for such
behaviour i.e. GMimeStream fseek()s in its write method to the position
recorded when the stream was created, so that in case there is somebody
else writing to the stream, the writes may overlap.

-Michal

Re: [PATCH] Setup the GMimeStream only when needed

[nstraz]

On Apr  1 16:21, Michal Sojka wrote:
> On Thu, 01 Apr 2010, nstraz@redhat.com wrote:
> > I ran into this while looking at the vim plugin.  Vim's system() call
> > redirects output to a file and it was missing many of the part{ lines.
> > 
> > If stream_stdout is setup too early, it will overwrite the part start
> > when notmuch is redirected to a file.
> 
> thanks for the patch. After some investigation, it seems that you are
> right. What I missed in your commit message is the reason for such
> behaviour i.e. GMimeStream fseek()s in its write method to the position
> recorded when the stream was created, so that in case there is somebody
> else writing to the stream, the writes may overlap.

Right, and in the case of writing to a pipe, the seek fails and the
file position doesn't change.  I found it comparing strace output of
`notmuch show ... > file` and `notmuch show ... | cat > file.`

Nate

Re: [PATCH] Setup the GMimeStream only when needed

[Carl Worth]

[-- Attachment #1: Type: text/plain, Size: 1182 bytes --]

On Thu, 1 Apr 2010 10:31:13 -0400, nstraz@redhat.com wrote:
> On Apr  1 16:21, Michal Sojka wrote:
> > On Thu, 01 Apr 2010, nstraz@redhat.com wrote:
> > thanks for the patch. After some investigation, it seems that you are
> > right. What I missed in your commit message is the reason for such
> > behaviour i.e. GMimeStream fseek()s in its write method to the position
> > recorded when the stream was created, so that in case there is somebody
> > else writing to the stream, the writes may overlap.
> 
> Right, and in the case of writing to a pipe, the seek fails and the
> file position doesn't change.  I found it comparing strace output of
> `notmuch show ... > file` and `notmuch show ... | cat > file.`

Ah, so the trigger of the bug is that we are currently interleaving
calls to printf with GMime writes to a stdout stream.

I'm happy to apply this patch to workaround the problem, (and I just did
push it with some of my own comments added to the commit message).

But isn't this a bug in GMime really? What possible use can it have for
doing an fseek on every stream write? That seems broken, and we should
report that to the GMime folks.

-Carl

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]