Hi, comments below.
2013-01-18 11:05 keltezéssel, Amit kapila írta:
On using mktemp, linux compilation gives below warning warning: the use of `mktemp' is dangerous, better use `mkstemp' So I planned to use mkstemp.
Good.
In Windows, there is an api _mktemp_s, followed by open call, behaves in a similar way as mkstemp available in linux. The code is changed to use of mkstemp functionality to generate a unique temporary file name instead of .lock file. Please check this part of code, I am not very comfortable with using this API.
I read the documentation of _mktemp_s() at http://msdn.microsoft.com/en-us/library/t8ex5e91%28v=vs.80%29.aspx The comment says it's only for C++, but I can compile your patch using the MinGW cross-compiler under Fedora 18. So either the MSDN comment is false, or MinGW provides this function outside C++. More research is needed on this. However, I am not sure whether Cygwin provides the mkstemp() call or not. Searching... Found bugzilla reports against mkstemp on Cygwin. So, yes, it does and your code would clash with it. In port/dirmod.c: ----8<--------8<--------8<--------8<--------8<---- #if defined(WIN32) || defined(__CYGWIN__) ... /* * pgmkstemp */ int pgmkstemp (char *TmpFileName) { int err; err = _mktemp_s(TmpFileName, strlen(TmpFileName) + 1); if (err != 0) return -1; return open(TmpFileName, O_CREAT | O_RDWR | O_EXCL, S_IRUSR | S_IWUSR); } /* We undefined these above; now redefine for possible use below */ #define rename(from, to) pgrename(from, to) #define unlink(path) pgunlink(path) #define mkstemp(tempfilename) pgmkstemp(tempfilename) #endif /* defined(WIN32) || defined(__CYGWIN__) */ ----8<--------8<--------8<--------8<--------8<---- pgmkstemp() should be defined in its own block inside #if defined(WIN32) && !defined(__CYGWIN__) ... #endif Same thing applies to src/include/port.h: ----8<--------8<--------8<--------8<--------8<---- #if defined(WIN32) || defined(__CYGWIN__) /* * Win32 doesn't have reliable rename/unlink during concurrent access. */ extern int pgrename(const char *from, const char *to); extern int pgunlink(const char *path); extern int pgmkstemp (char *TmpFileName); /* Include this first so later includes don't see these defines */ #ifdef WIN32_ONLY_COMPILER #include <io.h> #endif #define rename(from, to) pgrename(from, to) #define unlink(path) pgunlink(path) #define mkstemp(tempfilename) pgmkstemp(tempfilename) #endif /* defined(WIN32) || defined(__CYGWIN__) */ ----8<--------8<--------8<--------8<--------8<----
Removed the code which is used for deleting the .lock file in case of backend crash or during server startup.
Good.
Added a new function RemoveAutoConfTmpFiles used for deleting the temp files left out any during previous postmaster session in the server startup.
Good.
In SendDir(), used sizeof() -1
Good.
Since you have these defined:
+ #define PG_AUTOCONF_DIR "config_dir"
+ #define PG_AUTOCONF_FILENAME "postgresql.auto.conf"
+ #define PG_AUTOCONF_SAMPLE_TMPFILE "postgresql.auto.conf.XXXXXX"
+ #define PG_AUTOCONF_TMP_FILE "postgresql.auto.conf."
then the hardcoded values can also be changed everywhere. But to kill
them in initdb.c, these #define's must be in pg_config_manual.h instead of
guc.h.
Most notably, postgresql.conf.sample contains:
#include_dir = 'auto.conf.d'
and initdb replaces it with
include_dir = 'config_dir'.
I prefer the auto.conf.d directory name. After killing all hardcoded
"config_dir", changing one #define is all it takes to change it.
There are two blocks of code that Tom commented as being "over-engineered":
+ /* Frame auto conf name and auto conf sample temp file name */
+ snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR,
PG_AUTOCONF_FILENAME);
+ StrNCpy(AutoConfFileName, ConfigFileName, sizeof(AutoConfFileName));
+ get_parent_directory(AutoConfFileName);
+ join_path_components(AutoConfFileName, AutoConfFileName, Filename);
+ canonicalize_path(AutoConfFileName);
+
+ snprintf(Filename,sizeof(Filename),"%s/%s", PG_AUTOCONF_DIR,
PG_AUTOCONF_SAMPLE_TMPFILE);
+ StrNCpy(AutoConfTmpFileName, ConfigFileName,
sizeof(AutoConfTmpFileName));
+ get_parent_directory(AutoConfTmpFileName);
+ join_path_components(AutoConfTmpFileName, AutoConfTmpFileName,
Filename);
+ canonicalize_path(AutoConfTmpFileName);
You can simply do
snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_FILENAME);
snprintf(AutoConfTmpFileName, sizeof(AutoConfTmpFileName),"%s/%s/%s",
data_directory,
PG_AUTOCONF_DIR,
PG_AUTOCONF_SAMPLE_TMPFILE);
Also, mkstemp() and _mktemp_s() modify the file name in place,
so the XXXXXX suffix becomes something else. You can pass the
array pointer to write_auto_conf_file() together with the returned fd
so the ereport() calls can report the actual file name, not the template.
You removed the static keyword from before AbsoluteConfigLocation(),
resulting in this compiler warning:
In file included from guc.c:9274:0:
guc-file.l:382:1: warning: no previous prototype for 'AbsoluteConfigLocation'
[-Wmissing-prototypes]
In validate_conf_option() in guc.c, you had "return NULL;" and "return 0;" mixed. Since this function returns a pointer, I changed all to "return NULL;". void-returning function write_auto_conf_file() in guc.c had a "return;" at the end. It's not needed. I have fixed these in the attached patch and also changed wording of the documentation, some comments and the error messages to be more expressive or more concise. The difference between your patch and mine is also attached. Best regards, Zoltán Böszörményi -- ---------------------------------- Zoltán Böszörményi Cybertec Schönig & Schönig GmbH Gröhrmühlgasse 26 A-2700 Wiener Neustadt, Austria Web: http://www.postgresql-support.de http://www.postgresql.at/
set_persistent_v6.patch.gz
Description: Unix tar archive
set_persistent_v6_from_v5.patch.gz
Description: Unix tar archive
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
