Re: Win32: Minimising desktop heap usage - Mailing list pgsql-patches

Tom Lane wrote:
> Dave Page <dpage@postgresql.org> writes:
>> [ get rid of wsprintf in favor of snprintf ]
>
> +1 for not depending on nonstandard subroutines without need.
> But please note the standard locution is snprintf(buf, sizeof(buf), ...
> Not sizeof() - 1.

Noted.

>> !     char        *tmppath=0;
>
> Please use NULL not 0 ... actually, since the variable is
> unconditionally assigned to just below, I'd say this should just be
> "char *tmppath;".  I don't like useless initializations: if the compiler
> is not smart enough to discard them then they waste cycles, and they
> also increase the risk of bugs-of-omission in future changes.  If
> someone were to change things around so that tmppath didn't get assigned
> to in one code path, then the compiler would complain about use of an
> uninitialized variable --- *unless* you've written a useless
> initialization such as the above, in which case the mistake might pass
> unnoticed for awhile.  So don't initialize a local variable unless
> you're giving it an actually meaningful value.

The downside is that we see a useless warning that, if sufficiently
multiplied, might make it hard to see something we are interested in.

In this case, not initialising it will also create the first 'accepted'
warning in VC++ in our code :-(. All the others come from Kerberos.

Modified in the attached patch however.

>> !     /*
>> !      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
>> !      * will force us to link with shell32.lib which eats valuable desktop heap.
>> !      */
>> !     tmppath = getenv("APPDATA");
>
> Hmm, is there any functional downside to this?  I suppose
> SHGetSpecialFolderPath does some things that getenv does not...

A good percentage of the special folder paths you might query with
SHGetSpecialFolderPath() are not actually in the environment. APPDATA
certainly is on XP, 2K3 and Vista though, and I've found MS KB articles
referring to it being on 2K as well.

Regards, Dave.
Index: src/backend/port/win32/signal.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/port/win32/signal.c,v
retrieving revision 1.18
diff -c -r1.18 signal.c
*** src/backend/port/win32/signal.c    5 Jan 2007 22:19:35 -0000    1.18
--- src/backend/port/win32/signal.c    23 Oct 2007 14:44:35 -0000
***************
*** 178,184 ****
      char        pipename[128];
      HANDLE        pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
--- 178,184 ----
      char        pipename[128];
      HANDLE        pipe;

!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", (int) pid);

      pipe = CreateNamedPipe(pipename, PIPE_ACCESS_DUPLEX,
                         PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
***************
*** 251,257 ****
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%d", GetCurrentProcessId());

      for (;;)
      {
--- 251,257 ----
      char        pipename[128];
      HANDLE        pipe = pgwin32_initial_signal_pipe;

!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", GetCurrentProcessId());

      for (;;)
      {
Index: src/port/kill.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/kill.c,v
retrieving revision 1.8
diff -c -r1.8 kill.c
*** src/port/kill.c    5 Jan 2007 22:20:02 -0000    1.8
--- src/port/kill.c    23 Oct 2007 14:44:35 -0000
***************
*** 38,44 ****
          errno = EINVAL;
          return -1;
      }
!     wsprintf(pipename, "\\\\.\\pipe\\pgsignal_%i", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
--- 38,44 ----
          errno = EINVAL;
          return -1;
      }
!     snprintf(pipename, sizeof(pipename), "\\\\.\\pipe\\pgsignal_%u", pid);
      if (!CallNamedPipe(pipename, &sigData, 1, &sigRet, 1, &bytes, 1000))
      {
          if (GetLastError() == ERROR_FILE_NOT_FOUND)
Index: src/port/path.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/port/path.c,v
retrieving revision 1.71
diff -c -r1.71 path.c
*** src/port/path.c    5 Jan 2007 22:20:02 -0000    1.71
--- src/port/path.c    23 Oct 2007 14:47:29 -0000
***************
*** 628,637 ****
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        tmppath[MAX_PATH];

      ZeroMemory(tmppath, sizeof(tmppath));
!     if (SHGetFolderPath(NULL, CSIDL_APPDATA, NULL, 0, tmppath) != S_OK)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;
--- 628,643 ----
      strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
      return true;
  #else
!     char        *tmppath;

      ZeroMemory(tmppath, sizeof(tmppath));
!
!     /*
!      * Note: We use getenv here because the more modern SHGetSpecialFolderPath()
!      * will force us to link with shell32.lib which eats valuable desktop heap.
!      */
!     tmppath = getenv("APPDATA");
!     if (!tmppath)
          return false;
      snprintf(ret_path, MAXPGPATH, "%s/postgresql", tmppath);
      return true;