Hello Alvaro, thanks for the feedback, fixed all of your points.
Attached new version of patch.
--
Best regards,
Vladimir Kunschikov
Lead software developer
IDS project
InfoTeCS JSC
________________________________________
From: Alvaro Herrera <alvhe...@2ndquadrant.com>
Sent: Wednesday, July 26, 2017 1:02 AM
To: Kunshchikov Vladimir
Cc: pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Kunshchikov Vladimir wrote:
> Errors should be like this:
> pg_restore: [compress_io] could not read from input file: d3/2811.dat.gz:
> invalid distance too far back
>
> Attached small fix for this issue.
After looking at this patch, I don't like it very much. In particular,
I don't like the way you've handled the WRITE_ERROR_EXIT macro in
pg_backup_directory.c by undef'ing the existing one and creating it
anew. The complete and correct definition should reside in one place
(pg_backup_archiver.h), instead of being split in two and being defined
differently.
Another point is that you broke the comment on the definition of struct
cfp "this is opaque to callers" by moving it to the header file
precisely with the point of making it transparent to callers. We need
some better idea there. I think it can be done by making compress_io.c
responsible of handing over the error message through some new function
(something very simple like "if compressedfp then return get_gz_error
else strerror" should suffice).
Also, I needed to rebase your patch over a recent pgindent run, though
that's a pretty small change.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c
index 991fe11e8a..67908a069c 100644
--- a/src/bin/pg_dump/compress_io.c
+++ b/src/bin/pg_dump/compress_io.c
@@ -709,5 +709,25 @@ hasSuffix(const char *filename, const char *suffix)
suffix,
suffixlen) == 0;
}
+#endif
+const char *
+get_cfp_error(cfp* fp)
+{
+#ifdef HAVE_LIBZ
+ if(!fp->compressedfp)
+ return strerror(errno);
+
+ int errnum;
+
+ static const char fallback[] = "Zlib error";
+ const int maxlen = 255;
+ const char *errmsg = gzerror(fp->compressedfp, &errnum);
+ if(!errmsg || !memchr(errmsg, 0, maxlen))
+ errmsg = fallback;
+
+ return errnum == Z_ERRNO ? strerror(errno) : errmsg;
+#else
+ return strerror(errno);
#endif
+}
diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h
index 8f2e752cba..06b3762233 100644
--- a/src/bin/pg_dump/compress_io.h
+++ b/src/bin/pg_dump/compress_io.h
@@ -65,5 +65,6 @@ extern int cfgetc(cfp *fp);
extern char *cfgets(cfp *fp, char *buf, int len);
extern int cfclose(cfp *fp);
extern int cfeof(cfp *fp);
+extern const char * get_cfp_error(cfp *fp);
#endif
diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index 79922da8ba..e9c26bc571 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -352,7 +352,7 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen)
lclContext *ctx = (lclContext *) AH->formatData;
if (dLen > 0 && cfwrite(data, dLen, ctx->dataFH) != dLen)
- WRITE_ERROR_EXIT;
+ exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH));
return;
}
@@ -490,7 +490,7 @@ _WriteByte(ArchiveHandle *AH, const int i)
lclContext *ctx = (lclContext *) AH->formatData;
if (cfwrite(&c, 1, ctx->dataFH) != 1)
- WRITE_ERROR_EXIT;
+ exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH));
return 1;
}
@@ -519,7 +519,7 @@ _WriteBuf(ArchiveHandle *AH, const void *buf, size_t len)
lclContext *ctx = (lclContext *) AH->formatData;
if (cfwrite(buf, len, ctx->dataFH) != len)
- WRITE_ERROR_EXIT;
+ exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH));
return;
}
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index f839712945..f17ce78aca 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -35,6 +35,7 @@
#include "pgtar.h"
#include "common/file_utils.h"
#include "fe_utils/string_utils.h"
+#include "compress_io.h"
#include <sys/stat.h>
#include <ctype.h>
@@ -555,8 +556,12 @@ _tarReadRaw(ArchiveHandle *AH, void *buf, size_t len, TAR_MEMBER *th, FILE *fh)
{
res = GZREAD(&((char *) buf)[used], 1, len, th->zFH);
if (res != len && !GZEOF(th->zFH))
+ {
+ int errnum;
+ const char *errmsg = gzerror(th->zFH, &errnum);
exit_horribly(modulename,
- "could not read from input file: %s\n", strerror(errno));
+ "could not read from input file: %s\n", errnum == Z_ERRNO? strerror(errno) : errmsg);
+ }
}
else
{
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
