On Mon, Dec 13, 2010 at 07:38:37 -0200, Davi Arnaut wrote:
> Hi Vasil,
>
> On 12/8/10 10:29 AM, vasil.dimov@stripped wrote:
> > #At file:///bzrroot/server/mysql-5.1-innodb/ based on
> revid:vasil.dimov@stripped
> >
> > 3666 Vasil Dimov 2010-12-08
> > Fix Bug#51023 Mysql server crashes on SIGHUP and destroys InnoDB files
>
> Would be nice to have a concise description of the problem here.
Done.
> > Emulate freopen() on FreeBSD. This patch assumes we can assign to stdout
> > like "FILE* f; ...; stdout = f;"
> >
> > modified:
> > sql/log.cc
> > sql/mysqld.cc
> > === modified file 'sql/log.cc'
> > --- a/sql/log.cc revid:vasil.dimov@stripped
> > +++ b/sql/log.cc revid:vasil.dimov@stripped
> > @@ -5066,36 +5066,81 @@ void sql_perror(const char *message)
> > #endif
> > }
> >
> > +/**
> > + Opens filename and reassociates f with it.
> > +
> > + @param filename Path to file
> > + @param f Stream to reassociate
>
> No tabs.
Done. Btw I copied from test_if_number() which uses tabs.
> > + @note
> > + This function is used to redirect stdout and stderr to a file and
> > + subsequently to close and reopen that file for log rotation.
> > +
> > + @retval
> > + true Success
> > + @retval
> > + false Error
> > +*/
> > +static bool my_freopen(const char *filename, FILE** f)
>
> Associate ** with the name.
Done.
> > +{
> > +#ifdef __FreeBSD__
> > + int fd_new;
> > + int fd_orig;
> > + FILE* f_new;
> > +
> > + fd_new = open(filename, O_WRONLY | O_APPEND);
> > + if (fd_new == -1) {
>
> Curly bracket on the line below.
Done.
> > + fclose(*f);
> > + return false;
>
> Coding style mandates that true be returned on error.
Done.
> > + }
> > + fflush(*f);
> > + fd_orig = fileno(*f);
> > + if (dup2(fd_new, fd_orig) == -1) {
> > + close(fd_new);
> > + return false;
> > + }
> > + close(fd_new);
> > + f_new = fdopen(fd_orig, "a");
> > + if (f_new == NULL) {
> > + close(fd_orig);
> > + return false;
> > + }
> > + *f = f_new;
> > + return true;
> > +#else
> > + return freopen(filename, "a", *f) != NULL;
> > +#endif
> > +}
> >
> > #ifdef __WIN__
> > extern "C" my_bool reopen_fstreams(const char *filename,
> > - FILE *outstream, FILE *errstream)
> > + FILE **outstream, FILE **errstream)
> > {
> > int handle_fd;
> > int err_fd, out_fd;
> > HANDLE osfh;
> >
> > - DBUG_ASSERT(filename && errstream);
> > + DBUG_ASSERT(filename && *errstream);
> >
>
> Hum, I think we can simplify this function a bit in order to make this
> pointer to pointer passing nicer and reduce the amount of duplicated code:
>
> - Rename reopen_fstreams to repoen_fstream and make it take only a
> single FILE stream.
>
> - Make reopen_fstream and my_freopen return a FILE stream, much like
> freopen.
>
> - Add a my_reopen_stream wrapper that takes a FILE **. It passes the
> stream pointer (FILE *) to reopen_fstream, takes the returned pointer
> and sets it if on FreeBSD.
Lets do this simplification as a separate commit from the actual bugfix.
> > // Services don't have stdout/stderr on Windows, so _fileno returns -1.
> > - err_fd= _fileno(errstream);
> > + err_fd= _fileno(*errstream);
> > if (err_fd < 0)
> > {
> > - if (!freopen(filename, "a+", errstream))
> > + if (!my_freopen(filename, errstream))
>
> No need for my_freopen here? it's Windows only branch and we are not
> providing a Windows specific implementation.
Done.
Followup to:
http://lists.mysql.com/commits/126800
--
Vasil Dimov
moc.elcaro@stripped
%
Just because you are paranoid doesn't mean "they" aren't out to get you.
Attachment: [application/pgp-signature]
