SEO

The following patch adds appropriate error messages based on the errno
returned from rmtree() failures.

The original code came in this commit:

revision 1.13
date: 2004/08/01 06:19:26; author: momjian; state: Exp; lines: +173 -7
Add docs for initdb --auth.

which obviouisly had an inaccurate description. I am not even sure
where the rmtree() code came from, but I think it was Andrew Dunstan.

Anyway, it never had errno checks with messages, so we didn't strip it
out; rather it was never there. The original code did a
system("rmdir") call so this code was obviously superior.

Anyway, I will apply the attached patch to CVS HEAD and 8.0.X in a day
or so.

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Index: src/port/dirmod.c
================================================== =================
RCS file: /cvsroot/pgsql/src/port/dirmod.c,v
retrieving revision 1.34
diff -c -c -r1.34 dirmod.c
*** src/port/dirmod.c 31 Dec 2004 22:03:53 -0000 1.34
--- src/port/dirmod.c 12 Feb 2005 22:25:58 -0000
***************
*** 350,355 ****
--- 350,356 ----
return filenames;
}

+
/*
* fnames_cleanup
*
***************
*** 366,371 ****
--- 367,407 ----
pfree(filenames);
}

+
+ /*
+ * rmtree_errno
+ *
+ * report rmtree errno failure messages
+ */
+ static void
+ rmtree_errno(char *filepath)
+ {
+ if (errno == EACCES)
+ #ifndef FRONTEND
+ elog(LOG, "no permission to remove \"%s\"", filepath);
+ #else
+ fprintf(stderr, "no permission to remove \"%s\"", filepath);
+ #endif
+ else if (errno == EBUSY)
+ #ifndef FRONTEND
+ elog(LOG, "can not remove \"%s\" because it is in use", filepath);
+ #else
+ fprintf(stderr, "can not remove \"%s\" because it is in use", filepath);
+ #endif
+ else if (errno == ENOENT)
+ #ifndef FRONTEND
+ elog(LOG, "\"%s\" disappeared during directory removal", filepath);
+ #else
+ fprintf(stderr, "\"%s\" disappeared during directory removal", filepath);
+ #endif
+ else
+ #ifndef FRONTEND
+ elog(LOG, "can not remove \"%s\"", filepath);
+ #else
+ fprintf(stderr, "can not remove \"%s\"", filepath);
+ #endif
+ }
+
/*
* rmtree
*
***************
*** 399,404 ****
--- 435,441 ----

if (stat(filepath, &statbuf) != 0)
{
+ rmtree_errno(*filename);
fnames_cleanup(filenames);
return false;
}
***************
*** 416,421 ****
--- 453,459 ----
{
if (unlink(filepath) != 0)
{
+ rmtree_errno(*filename);
fnames_cleanup(filenames);
return false;
}
***************
*** 426,431 ****
--- 464,470 ----
{
if (rmdir(path) != 0)
{
+ rmtree_errno(*filename);
fnames_cleanup(filenames);
return false;
}


---------------------------(end of broadcast)---------------------------
TIP 7: don't forget to increase your free space map settings

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> + static void
> + rmtree_errno(char *filepath)
> + {
> + if (errno == EACCES)
> + #ifndef FRONTEND
> + elog(LOG, "no permission to remove \"%s\"", filepath);
> + #else
> + fprintf(stderr, "no permission to remove \"%s\"", filepath);
> + #endif
> + else if (errno == EBUSY)
> [ etc ]

This seems awfully bogus: it's incomplete and not in the style of our
other error messages. Why not just one case:

#ifndef FRONTEND
elog(LOG, "could not remove \"%s\": %m", filepath);
#else
fprintf(stderr, "could not remove \"%s\": %s\n", filepath,
strerror(errno));
#endif

Also, I'm unconvinced that LOG is the appropriate error level;
WARNING would probably be better.

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 2: you can get off all lists at once with the unregister command
(send "unregister YourEmailAddressHere" to majordomo@postgresql.org)

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > + static void
> > + rmtree_errno(char *filepath)
> > + {
> > + if (errno == EACCES)
> > + #ifndef FRONTEND
> > + elog(LOG, "no permission to remove \"%s\"", filepath);
> > + #else
> > + fprintf(stderr, "no permission to remove \"%s\"", filepath);
> > + #endif
> > + else if (errno == EBUSY)
> > [ etc ]
>
> This seems awfully bogus: it's incomplete and not in the style of our
> other error messages. Why not just one case:
>
> #ifndef FRONTEND
> elog(LOG, "could not remove \"%s\": %m", filepath);
> #else
> fprintf(stderr, "could not remove \"%s\": %s\n", filepath,
> strerror(errno));
> #endif

OK, so you just wanted _an_ error string to be printed on failure rather
than the reasons.

> Also, I'm unconvinced that LOG is the appropriate error level;
> WARNING would probably be better.

Yea, good point.

--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

---------------------------(end of broadcast)---------------------------
TIP 9: the planner will ignore your desire to choose an index scan if your
joining column's datatypes do not match