On Fri, Jul 15, 2011 at 15:03, MauMau <maumau...@gmail.com> wrote: > Magnus, > > Thank you for reviewing my patch. I'm going to modify the patch according to > your comments and re-submit it. Before that, I'd like to discuss some points > and get your agreement.
Ok, please do. If you want to, you can work off my git branch at http://github.com/mhagander/postgres (branch multievent). > From: "Magnus Hagander" <mag...@hagander.net> >> >> + <para> >> + On Windows, you need to register an event source >> + and its library with the operating system in order >> + to make use of the <systemitem>eventlog</systemitem> option for >> + <varname>log_destination</>. >> + See <xref linkend="event-log-registration"> for details. >> + </para> >> >> * This part is not strictly correct - you don't *need* to do that, it >> just makes things look nicer, no? > > How about the following statement? Is it better to say "correctly" instead > of "cleanly"? > > -------------------------------------------------- > On Windows, when you use the eventlog option for log_destination, you need > to register an event sourceand its library with the operating system so that > the Windows Event Viewer can display event log messages cleanly. > -------------------------------------------------- Replace "need" with "should" and I'm happy with that. >> * Also, what is the use for set_eventlog_parameters()? It's just a >> string variable, it shuold work without it. > > Yes, exactly. I'll follow your modification. > >> * We these days avoid #ifdef'ing gucs just because they are not on >> that platform - so the list is consistent. The guc should be available >> on non-windows platforms as well. > > I see. When I saw syslog_ident not being #ifndef WIN32'ed, I thought that > was because syslog might be available on Cygwin or MinGW. Anyway, I'll take > your modification. Nope, it used to be #ifdefed on HAVE_SYSLOG, but we changed that a while ago for consistency. >> * The guc also needs to go in postgresql.conf.sample > > I missed this completely. > >> * Are we really allowed to call MessageBox in DlLRegisterService? >> Won't that break badly in cases like silent installs? > > It seems that the only way to notify the error is MessageBox. Printing to > stdout/stderr does not show any messages on the command prompt. I guess > that's why the original author of pgevent.c used MessageBox. oh, we're already using messagebox.. I must've been confused, I thought it was new. Heck, it might even be me who wrote that :O > We cannot know within the DLL if /s (silent) switch was specified to > regsvr32.exe. If we want to suppress MessageBox during silent installs, it > seems that we need to know the silent install by an environment variable or > so. That is: > > C:\> set PGSILENT=true > C:\> regsvr32.exe ... I think if we're not Ok with messagebox, then we should just write it to the eventlog. However, given that we have been using messagebox before and had no complaints, I should withdraw my complaint for now - keep using messagebox like the surrounding code. We can attack the potential issue of that in a separate patch later. >> * We never build in unicode mode, so all those checks are unnecessary. >> >> Attached is an updated patch, which doesn't work yet. I believe the >> changes to the backend are correct, but probably some of the cleanups >> and changes in the dll are incorrect, because I seem to be unable to >> register either the default or a custom handler so far. > > The cause of the problem you are encountering is here: > > + if (pszCmdLine && *pszCmdLine != '\0') > + strncpy(event_source, sizeof(event_source), pszCmdLine); > + else > + strcpy(event_source, "PostgreSQL"); > > DllInstall() always receives the /i argument in UTF-16. str... functions > like strncpy() cannot be used for handling UTF-16 strings. For example, when > you run "regsvr32.exe /i:abc ...", DllInstall's pszCmdLine value becomes > "a\0b\0c\0". So, strncpy() just copies "a". This is the reason why you > cannot register the custom event source. Oh, it gets it as UTF-16 even when not in unicode mode. I see - yeah, I didn't have time to read up on the calling conventions properly. > The reason why you cannot register the default event source (i.e. running > regsvr32 without /i) is that you memset()ed event_source in DllMain() and > set "PostgreSQL" to it in DllInstall(). DllInstall() is not called when /i > is not specified. So, event_source remains empty. > > To solve the problem, we need to use wcstombs_s() instead of strncpy(), and > initialize event_source to "PostgreSQL" when it is defined. Ok, seems reasonable. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers