On 04/07/2011 02:50 PM, Denys Vlasenko wrote:

On Wed, 2011-04-06 at 17:46 +0200, Jiri Moskovcak wrote:

...

we can't use /var/run/abrt as a tmp directory when we're not running under root, so I changed the code to use /tmp instead.

• char tmpdir_name[] = "/tmp/rhtsupport-XXXXXX";
• /* mkdtemp does mkdir(xxx, 0700), should be safe (is it?) */
• if (mkdtemp(tmpdir_name) == NULL)
• {

•    error_msg_and_die(_("Can't create a temporary directory in /tmp"));
  

• }
• tempfile = xasprintf("%s/tmp-%s-%lu.tar.gz",tmpdir_name, iso_date_string(NULL), (long)getpid());

Think how /tmp/ will look after it will accumulate several stale /tmp/rhtsupport-XXXXXX's:

/tmp/rhtsupport-109824 /tmp/rhtsupport-092456 /tmp/rhtsupport-239048 /tmp/rhtsupport-987252 /tmp/rhtsupport-932148 ...

They look all the same. Which ones are old? How old? Which are outdated and thus the safest to delete? etc...

I propose to add date to the directory name too. Something like this:

• const char *dt_string = iso_date_string(NULL);
• char tmpdir_name[sizeof("/tmp/rhtsupport-YYYY-MM-DD-hh:mm:ss-XXXXXX")];
• sprintf(tmpdir_name, "/tmp/rhtsupport-%s-XXXXXX", dt_string);
• /* mkdtemp does mkdir(xxx, 0700), should be safe (is it?) */
• if (mkdtemp(tmpdir_name) == NULL)
• {

•    error_msg_and_die(_("Can't create a temporary directory in /tmp"));
  

• }
• tempfile = xasprintf("%s/tmp-%s-%lu.tar.gz", tmpdir_name, dt_string, (long)getpid());

Apart from this proposal, patch look good to me.

- the patch removes the tmp directory when it's done with it, but it can't hurt if we make the name more "search friendly", I'll change it.

Thank you for the review.