Thread: Errno checks for rmtree()
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;
}
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
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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> OK, so you just wanted _an_ error string to be printed on failure rather
> than the reasons.
Well, what we want to know is the particular file that couldn't be
deleted and the errno code. This is all "can't happen" stuff and so
it just has to be a message that is useful for debugging; I don't see
that it has to be amazingly user-friendly.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > OK, so you just wanted _an_ error string to be printed on failure rather > > than the reasons. > > Well, what we want to know is the particular file that couldn't be > deleted and the errno code. This is all "can't happen" stuff and so > it just has to be a message that is useful for debugging; I don't see > that it has to be amazingly user-friendly. Wow, you want to print out the raw errno number. We only do that in one or two places in our code that I could fine. -- 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
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Wow, you want to print out the raw errno number.
No, I didn't say that. I wanted the strerror result, and that's what
the code I suggested does.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Wow, you want to print out the raw errno number. > > No, I didn't say that. I wanted the strerror result, and that's what > the code I suggested does. OK, new version. -- 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 13 Feb 2005 01:51:35 -0000 *************** *** 350,355 **** --- 350,356 ---- return filenames; } + /* * fnames_cleanup * *************** *** 366,371 **** --- 367,373 ---- pfree(filenames); } + /* * rmtree * *************** *** 398,413 **** snprintf(filepath, MAXPGPATH, "%s/%s", path, *filename); if (stat(filepath, &statbuf) != 0) ! { ! fnames_cleanup(filenames); ! return false; ! } if (S_ISDIR(statbuf.st_mode)) { /* call ourselves recursively for a directory */ if (!rmtree(filepath, true)) { fnames_cleanup(filenames); return false; } --- 400,413 ---- snprintf(filepath, MAXPGPATH, "%s/%s", path, *filename); if (stat(filepath, &statbuf) != 0) ! goto report_and_fail; if (S_ISDIR(statbuf.st_mode)) { /* call ourselves recursively for a directory */ if (!rmtree(filepath, true)) { + /* we already reported the error */ fnames_cleanup(filenames); return false; } *************** *** 415,436 **** else { if (unlink(filepath) != 0) ! { ! fnames_cleanup(filenames); ! return false; ! } } } if (rmtopdir) { if (rmdir(path) != 0) ! { ! fnames_cleanup(filenames); ! return false; ! } } fnames_cleanup(filenames); return true; } --- 415,440 ---- else { if (unlink(filepath) != 0) ! goto report_and_fail; } } if (rmtopdir) { if (rmdir(path) != 0) ! goto report_and_fail; } fnames_cleanup(filenames); return true; + + report_and_fail: + + #ifndef FRONTEND + elog(WARNING, "can not remove \"%s\": %s", filepath, strerror(errno)); + #else + fprintf(stderr, "can not remove \"%s\": %s\n", filepath, strerror(errno)); + #endif + fnames_cleanup(filenames); + return false; }
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> + elog(WARNING, "can not remove \"%s\": %s", filepath, strerror(errno));
"could not remove", please; read the message style guidelines.
Also, what's wrong with using %m here?
Otherwise it looks good.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > + elog(WARNING, "can not remove \"%s\": %s", filepath, strerror(errno)); > > "could not remove", please; read the message style guidelines. > Also, what's wrong with using %m here? > > Otherwise it looks good. OK, new version attached. -- 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 13 Feb 2005 02:23:22 -0000 *************** *** 350,355 **** --- 350,356 ---- return filenames; } + /* * fnames_cleanup * *************** *** 366,371 **** --- 367,373 ---- pfree(filenames); } + /* * rmtree * *************** *** 398,413 **** snprintf(filepath, MAXPGPATH, "%s/%s", path, *filename); if (stat(filepath, &statbuf) != 0) ! { ! fnames_cleanup(filenames); ! return false; ! } if (S_ISDIR(statbuf.st_mode)) { /* call ourselves recursively for a directory */ if (!rmtree(filepath, true)) { fnames_cleanup(filenames); return false; } --- 400,413 ---- snprintf(filepath, MAXPGPATH, "%s/%s", path, *filename); if (stat(filepath, &statbuf) != 0) ! goto report_and_fail; if (S_ISDIR(statbuf.st_mode)) { /* call ourselves recursively for a directory */ if (!rmtree(filepath, true)) { + /* we already reported the error */ fnames_cleanup(filenames); return false; } *************** *** 415,436 **** else { if (unlink(filepath) != 0) ! { ! fnames_cleanup(filenames); ! return false; ! } } } if (rmtopdir) { if (rmdir(path) != 0) ! { ! fnames_cleanup(filenames); ! return false; ! } } fnames_cleanup(filenames); return true; } --- 415,440 ---- else { if (unlink(filepath) != 0) ! goto report_and_fail; } } if (rmtopdir) { if (rmdir(path) != 0) ! goto report_and_fail; } fnames_cleanup(filenames); return true; + + report_and_fail: + + #ifndef FRONTEND + elog(WARNING, "could not remove file or directory \"%s\": %m", filepath); + #else + fprintf(stderr, "could not remove file or directory \"%s\": %s\n", filepath, strerror(errno)); + #endif + fnames_cleanup(filenames); + return false; }