bugPSPP - Bugs: bug #33483, temporary directory

 
 

bug #33483: temporary directory

Submitter:  Harry Thijssen <unknown_one>
Submitted:  Mon 06 Jun 2011 12:20:07 PM UTC
   
 
Category:  System/Portable File I/O Severity:  5 - Average
Status:  Fixed Assigned to:  None
Open/Closed:  Closed Release:  Before 0.8.0
Effort:  0.00
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 25 Aug 2011 06:07:13 PM UTC, comment #17: 

The temporary directory is correctly created in the dir3ectory pointed to by temp. However, the create dir is not removed when closing psppire.

Harry Thijssen <unknown_one>
Tue 23 Aug 2011 06:47:00 PM UTC, comment #16: 

The gnulib people have pushed the patch we sent - or one similar to it. 

So this issue is hopefully fixed now.

John Darrington <jmd>
Group administrator
Fri 01 Jul 2011 05:47:00 PM UTC, comment #15: 

The problems relating to deleting the directory have now been fixed too.

John Darrington <jmd>
Group administrator
Sat 25 Jun 2011 09:33:12 PM UTC, comment #14: 

I generated a new MSWindows build with this patch. It can be found at http://sourceforge.net/projects/pspp4windows/files/2011-06-25-only-for-testing/

It seems to work, but needs more testing. Especially on network environments like in universities. I have no ability to test this.

Test reports are welcome. 

If this version fails, which I don't expect, you can always go back to the previous version by just deleting your c:\Program files\PSPP or whatever it is on your computer and reinstall the previous PSPP version.

Harry Thijssen <unknown_one>
Sat 25 Jun 2011 06:47:26 PM UTC, comment #13: 

ok.  Can you try this revised patch.

It uses the the windows function in place of getenv.  I've also
replaced "/Windows/Temp" with "\\", so that'll be the last resort
fallback.

Thanks.

(file #23559)

John Darrington <jmd>
Group administrator
Sat 25 Jun 2011 02:54:30 PM UTC, comment #12: 

As far as I understand:

#include <stdio.h>
#ifndef P_tmpdir
# define P_tmpdir "/tmp"
#endif

leaves P_tmpdir filled by stdio.h or defined it with "/tmp". In stdio.h it is defined as "\" actually "\\". To me it looks as if it never tries to use /tmp.

A hardcoded fallback to /tmp has indeed the same issues as /Windows/Temp. Even more as /tmp is not defined by MSWindows.

The MSWindows syscall looks promising. Surprising that MS in manuals states  that TMP is old and programs use the newer TEMP and in such a call TMP is first checked. !@#$%^&





Harry Thijssen <unknown_one>
Sat 25 Jun 2011 02:29:12 PM UTC, comment #11: 

The current (unmodified gnulib) behaviour is to use $TMPDIR, then "/tmp", then abort with an error.

My patch, on Windows, changes it to use $TEMP, then "/Windows/Temp", then abort with error.

So far as I can see, your patch uses $TEMPDIR, then $TEMP, then "/tmp", then abort with error.

Surely a fallback of "/tmp" has the same problems (or worse) than "/Windows/Temp" ?

Perhaps the correct solution would be to use the Windows syscall
GetTempPath, see http://msdn.microsoft.com/en-us/library/aa364992%28v=vs.85%29.aspx  I presume this will be more robust, as it should know about all the Windows specific idiosyncrases.

Shall we have a go at that?

John Darrington <jmd>
Group administrator
Sat 25 Jun 2011 01:54:45 PM UTC, comment #10: 

I see.

I am afraid for the fallback directory. MSWindows doesn't use 1 file system with \ as root but it may have several roots like C:\ D:\ etc. so \Windows\Temp maybe available on another drive as the current.

In the current situation there is a working fallback as this is always used for instance for PSPP on MSWindows systems.

Your patch solves the TEMP issue and is hopefully acceptable for the gnulib maintainer. This will be a benefit for all people porting GNU software to MSWindows.

But I have my doubts about the second part. The current code works and I am not so sure about the changed code. This needs extensive testing.
After the TEMP issue is solved, almost knowbody will use the fallback TMP path as it is very unusual not to have a correct path specified in TEMP.

My stomach says to me use the first part for the TEMP change and leave the code for the fallback path as it is. You won't gain much with it and introduce a risk.  


What do you think about it? 


Harry Thijssen <unknown_one>
Fri 24 Jun 2011 05:56:58 PM UTC, comment #9: 

The gnulib maintainer is extremely conservative.   I suspect he will reject a patch which makes  POSIX systems use the "TEMP" variable.  Likewise, a patch which duplicates the code (even for only 6 lines) is likely to be treated with disdain.

Of course, I can't guarantee that my patch will be accepted, but from what I've experienced on the gnulib mailing list so far, I feel it has a greater chance.

If you think that "/" is better than "/Windows/Temp", then I can change that accordingly.

John Darrington <jmd>
Group administrator
Fri 24 Jun 2011 05:00:20 PM UTC, comment #8: 

Hi

Since the beginning of this month I use the patch at http://sourceforge.net/projects/pspp4windows/files/2011-06-07/used-to-build/tmpdir.man-patch/ for the MSWindows build.
It must be downloaded from sourceforge about a 1000 times yet so this is reasonable stable.
Is your goal to have a reasonable place even if TEMP is not defined as an environment variable?
In the original code this would  mean it is place on the root which is always available (and writable as I found out!!) and this was the previous behavior and I haven't seen a problem report about that. Using \Windows\Temp looks tricky to me.

Harry Thijssen <unknown_one>
Fri 24 Jun 2011 02:49:34 PM UTC, comment #7: 

Harry,

Can you try the attached patch (against gnulib).  It should fix the location of the tempdir under windows.  Can you make sure it behaves how windows users would like, even when no TEMP variable is set - I've coded it to fallback to /Windows/Temp which I think is common for windows.  Perhaps you know better.  If you have any suggestions, please add them.

When we've got this patch working optimally, I'll submit it to gnulib.

This doesn't fix the not-being-deleted issue.  We'll deal with that later.

(file #23553)

John Darrington <jmd>
Group administrator
Fri 10 Jun 2011 01:32:42 PM UTC, comment #6: 

Now that you mention it, the temp directories are not getting removed on debian either.

I think there are multiple causes.  The temp dir fails to be removed both in the case where pspp exits normally and upon SIGINT.


Looking at the pspp source, I notice a couple of things:

  • In src/libpspp/ext-array.c line 72 the call to fclose should

  instead be a call to close_temp_file

  • In src/libpspp/temp-file.c the function create_temp_file is

  missing a call to register_temp_file


I made these changes and found that the temp dir is then removed
upon SIGINT, but not on normal exit.

  • Looking in the gnulib source lib/clean-temp.c I can see a line

  of the form at_fatal_signal (cleanup) but nothing to indicate
  that cleanup will be called on a normal exit (although the
  comment in clean-temp.h says that it will


John Darrington <jmd>
Group administrator
Fri 10 Jun 2011 10:59:19 AM UTC, comment #5: 

I checked this on openSUSE, and pspp leaves their the temporary directories behind too. After a while this is a large amount of disk space occupied by temporary directories which are not needed after pspp is closed.

I would say if pspp creates a temporary directory it should remove it too.

Harry Thijssen <unknown_one>
Tue 07 Jun 2011 12:32:35 PM UTC, comment #4: 

I just released a PSPP for MSWindows package which includes this patch. However, it is better if it is improved in teh gnulib libraries as this will effect other packages too. So contacting the gnulib list would be great.

Harry Thijssen <unknown_one>
Tue 07 Jun 2011 09:25:36 AM UTC, comment #3: 

TMPDIR is indeed very unusual on MSWindows. TEMP is the preferred variable.

I am thinking of using a temporary patch for this in the MSWindows package.

I guess changing:
  if (try_tmpdir)
    {
      d = __secure_getenv ("TMPDIR");
      if (d != NULL && direxists (d))
        dir = d;
      else if (dir != NULL && direxists (dir))
        /* nothing */ ;
      else
        dir = NULL;
    }
to
    {
      d = __secure_getenv ("TMPDIR");
      if (d != NULL && direxists (d))
        dir = d;
      else {
        d = __secure_getenv ("TEMPDIR");
        if (d != NULL && direxists (d))
          dir = d;
        else if (dir != NULL && direxists (dir))
          /* nothing */ ;
        else
          dir = NULL;
       }
    }
at line 99 would fix the issue of the place where the directory is created. But this leaves the issue of the large files not being removed. 1 user had 3,4 Gb temporary files stored. If this is on the TEMP directory, fortunately the directory is cleaned by several garbage collection tools like the one form MSWindows itself.

I think it would be good to ask the gnulib list to add the TEMP variable and also ask why the files are not deleted. Maybe it is just a minor issue.

Harry Thijssen <unknown_one>
Tue 07 Jun 2011 08:10:39 AM UTC, comment #2: 

The code which implements this comes from gnulib.
See http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/tmpdir.c

The comment there says that is respects the TMPDIR environment variable, but doesn't mention TMP or TEMP which I think are common conventions in the Windows world.  Perhaps a polite request on the gnulib mailing list might be appropriate if we think it should respect either of these.

Why the directory is not being deleted, I don't know.  Perhaps it depends upon a signal which is not available in the windows.

John Darrington <jmd>
Group administrator
Mon 06 Jun 2011 02:37:59 PM UTC, comment #1: 

The directory is created whenever a very large file is processed with PSPPIRE. I didn't check for PSPP.

Harry Thijssen <unknown_one>
Mon 06 Jun 2011 12:20:07 PM UTC, original submission:  

When testing for the bug #33250 I found that, on MSWindows, there is a directory, called pspp<something random> created at C:\ This directory is not removed after psppire is ending.

Besides, it would be better if them temp directory was used for this. temp and tmp where both defined, so this can't be the reason why the files are created on the root.


Harry Thijssen <unknown_one>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #23559:  tmpdir2.patch added by jmd (2KiB - text/x-patch)
file #23553:  windows-tempdir.patch added by jmd (1KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jmd (Posted a comment)
  • -email is unavailable- added by unknown_one (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2011-08-23 jmd StatusNone Fixed
        Open/ClosedOpen Closed
    2011-06-25 jmd Attached File- Added tmpdir2.patch, #23559
    2011-06-24 jmd Attached File- Added windows-tempdir.patch, #23553
    2011-06-10 jmd ReleaseNone Before 0.8.0

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code