GNOME Bugzilla – Bug 108659
Plug-ins should use g_spawn instead of popen
Last modified: 2005-03-02 02:34:36 UTC
popen is quite idiosyncratic across platforms, plus (at least on Unix) involves the shell, leading to subtle bugs. The ps, mail, and print plug-ins should use gspawn so we don't need to worry about such things.
Changing milestone to 2.0 as this is really minor problem and could be fixed in the 2.0 releases.
With the removal of EMX support this should have become a good deal simpler. Would be nice to see this happening still but it's not a blocker.
Removing popen() needs to add mknod() and something like tempnam() to get a name for the named pipe. mkstemp() or g_mkstemp() can not be used, because they open an ordinary file. Is mknod() and tempnam() available on all systems where popen() is already running ?
No, we shouldn't be using named pipes, but the GSpawn API, which wraps the anonymous pipe creation for us in a portable fashion. http://developer.gnome.org/doc/API/2.0/glib/glib-Spawning-Processes.html The main problem with popen is that it uses /bin/sh, which means have all the shell quoting problems to deal with.
Created attachment 23715 [details] [review] patch to use g_spawn API in plug-in ps
The patch is against pre1-tarball. Tested on Linux. Needs to be tested on Windows, specially with input files that contain blanks etc. Every quoting of input files has been removed from the plug-in. This should now be done by g_spawn...
Tor, can you test this?
In your patch Peter, could you consider using a GPtrArray? It's more readable that way, imho. Also, with GSpawn, can win32 can handle using a pipe and not a temp file, right?
The code has a comment "For instance, the Win32 port of ghostscript doesn't work correctly when using standard output as output file." I don't remember what the exact problems were, and don't know whether that still is true with current ghostscript versions. But it probably is safer to keep using the temp file on Win32, even if it of course would make the code cleaner if one could get rid of the USE_REAL_OUTPUTFILE ifdefs. I'll check...
Doh, silly me, I just looked at the patch and not the source code itself. In this case, I agree it's safer to keep using a temp file, because I'm sure there are users with old ghostscript versions out there, even if this issue is resolved in newer ones.
Created attachment 23730 [details] [review] Patch to use GPtrArray. Patches patched version.
A patch against the patched version makes it difficult to review and to apply the change. Could you create a diff against CVS instead?
Hi, I just uploaded the patch to use GPtrArray. About the problem with ghostscript using pipes on Windows. I also don't remember why I added this. I just did some tests and saw that when starting ghostscript, I need to add the -dBATCH. Otherwise ghostscript does not finish on Windows. I will add later another patch that includes this. Currently I have to leave home so that I can't do it now.
OK, I will add a new patch that is based on the CVS-version. And it also will include the -dBATCH. Takes some hours. I must leave now.
Created attachment 23737 [details] [review] Patch against CVS for gspawn API, -dBATCH
Last patch is against pre1-tarball, uses g_spawn API, GPtrArray, -dBATCH.
Looks good, I'll clean it up and commit it.
2004-01-25 Manish Singh <yosh@gimp.org> * plug-ins/common/postscript.c: use GSpawn instead of popen, -dBATCH, fixes #108659 for this plugin. Thanks to Peter Kirchgessner for the patch. Btw, Peter, g_ptr_array_free doesn't free the pointed to memory, so I used g_strfreev for that.
So that's mail and print left, both probably quite simple to do. The mail plug-in is quite useless and a port of the print plug-in to a newer version of gimpprint is scheduled for 2.0.1 or 2.0.2. IMO this bug isn't blocking the 2.0 release and could very well be moved to the 2.0.1 milestone already.
Agreed. We should look at the print plugin when we port to their new API, and mail is pretty trivial and not that critical.
Bumping to milestone 2.2. As long as it works, there's no good reason why this should be changed in the stable branch.
This shouldn't block the 2.2 release.
Fixed the mail plugin: 2005-03-01 Manish Singh <yosh@gimp.org> * plug-ins/common/mail.c: use g_spawn_async_with_pipes instead of popen. Addresses bug #108659. Also some general cleanup.
I'm going to close this bug. Newer gimp-print wraps the functionality the print code into it's ui library, so once we use the new library, this is no longer our issue.