List:Commits« Previous MessageNext Message »
From:Vasil Dimov Date:December 14 2010 4:12pm
Subject:Re: bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666)
Bug#51023
View as plain text  
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]
Thread
bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666) Bug#51023vasil.dimov8 Dec
  • Re: bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666) Bug#51023Davi Arnaut13 Dec
    • Re: bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666)Bug#51023Vasil Dimov14 Dec
      • Re: bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666) Bug#51023Davi Arnaut14 Dec
        • Re: bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666)Bug#51023Vasil Dimov15 Dec
  • Re: bzr commit into mysql-5.1-innodb branch (vasil.dimov:3666) Bug#51023Davi Arnaut13 Dec